add converter parameter to TokenWrapper for WETH9-compatible interfac… [IWrapper v1.0.0, TokenWrapper v1.2.0] #1531

ezynda3 · 2025-12-11T15:00:45Z

…e support

Which Jira task belongs to this PR?

Why did I implement it this way?

Checklist before requesting a review

I have performed a self-review of my code
This pull request is as small as possible and only tackles one problem
I have added tests that cover the functionality / test the bug
For new facets: I have checked all points from this list: https://www.notion.so/lifi/New-Facet-Contract-Checklist-157f0ff14ac78095a2b8f999d655622e
I have updated any required documentation

Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)

I have checked that any arbitrary calls to external contracts are validated and or restricted
I have checked that any privileged calls (i.e. storage modifications) are validated and or restricted
I have ensured that any new contracts have had AT A MINIMUM 1 preliminary audit conducted on by <company/auditor>

…e support

coderabbitai · 2025-12-11T15:00:58Z

Walkthrough

The pull request integrates converter address handling into the TokenWrapper contract and updates deployment scripts to support optional token converters. It adds converter configuration to deployment requirements, includes converter-aware initialization logic, and expands test coverage with new converter test utilities.

Changes

Cohort / File(s)	Summary
TokenWrapper Contract Core `src/Periphery/TokenWrapper.sol`	Updated license to LGPL-3.0-only; replaced `wrappedToken` with immutable `WRAPPED_TOKEN`; added `CONVERTER`, `USE_CONVERTER`, and `SWAP_RATIO_MULTIPLIER` immutables; expanded constructor signature to include `_converter` parameter; refactored `deposit()` and `withdraw()` to support converter-based conversion paths; updated version to 1.2.0.
Wrapper Interface `src/Interfaces/IWrapper.sol`	New interface file defining `deposit()` and `withdraw()` function signatures with LGPL-3.0-only license.
Deployment Scripts - Converter Integration `script/deploy/facets/DeployTokenWrapper.s.sol`, `script/deploy/zksync/DeployTokenWrapper.zksync.s.sol`	License updated to LGPL-3.0-only; added logic to read `converterAddress` from config files with try/catch fallback to `address(0)`; updated constructor args encoding to include three elements: `wrappedNativeAddress`, `converterAddress`, `refundWalletAddress`; added contract verification for converter (Facets only).
Tron Deployment Script `script/deploy/tron/deploy-and-register-periphery.ts`	Integrated `converterAddress` handling from Tron config; computed `converterHex` from config or default to zero address; updated TokenWrapper deployment sections to include converter in constructor args; added logging for deployed converter address.
Deployment Configuration `script/deploy/resources/deployRequirements.json`	Added new `TokenWrapper` entry with three config parameters: `_wrappedToken`, `_converter`, and `_owner`; `_converter` allows zero address deployment.
Converter Configuration `config/tokenconverters.json`	New configuration file with stable token converter address.
Test Converter Utilities `test/solidity/utils/TestWrappedConverter.sol`, `test/solidity/utils/TestConverterWithDecimals.sol`, `test/solidity/utils/TestBasicToken.sol`	Three new test utility contracts: `TestWrappedConverter` (1:1 wrapping/unwrapping), `TestConverterWithDecimals` (18-to-6 decimal conversion), and `TestBasicToken` (basic ERC20 with mint/burn).
TokenWrapper Test Suite `test/solidity/Periphery/TokenWrapper.t.sol`	License updated to LGPL-3.0-only; renamed tests to snake_case; added three test scenarios (basic, converter, decimal-converter); expanded test state with converter instances and tokens; added dedicated tests for converter paths: `test_CanDepositWithConverter`, `test_CanWithdrawWithConverter`, `test_ConverterReceivesApproval`, `test_CanDepositWithDecimalConverter`, `test_CanWithdrawWithDecimalConverter`, `test_DecimalConverterRoundTrip`.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

TokenWrapper constructor logic: Verify correct initialization of USE_CONVERTER and SWAP_RATIO_MULTIPLIER calculation based on converter decimals.
Converter integration in deposit/withdraw: Ensure conditional branching between converter and direct paths is correct and handles edge cases (zero address, transfer failures).
Deployment script consistency: Cross-check that all three deployment scripts (Facets, zkSync, Tron) correctly read and pass converter addresses with proper fallbacks.
Test coverage alignment: Validate that new test utilities accurately simulate converter behavior for validation of main contract logic.

Possibly related PRs

Tron deployments #1303: Updates Tron periphery deployment flows and TokenWrapper deployment logic, directly overlapping with the converter address integration in deployment scripts.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The PR description is incomplete. Key sections are missing: the Jira task reference and implementation rationale are not filled in, and the reviewer checklist items are unchecked.	Complete the description by filling in the Jira task field and rationale for implementation, and verify that reviewer checklist items are addressed before merge.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: adding a converter parameter to TokenWrapper with WETH9-compatible interface support, including version information.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch feat/token-wrapper-converter

Warning

Review ran into problems

🔥 Problems

Errors were encountered while retrieving linked issues.

Errors (1)

JIRA integration encountered authorization issues. Please disconnect and reconnect the integration in the CodeRabbit UI.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

src/Periphery/TokenWrapper.sol

lifi-action-bot · 2025-12-11T15:05:59Z

Test Coverage Report

Line Coverage: 85.34% (3004 / 3520 lines)
Function Coverage: 89.09% ( 482 / 541 functions)
Branch Coverage: 64.01% ( 482 / 753 branches)
Test coverage (85.34%) is above min threshold (83%). Check passed.

coderabbitai

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/Periphery/TokenWrapper.sol (1)

1-2: Update core periphery contract SPDX to match src/**/*.sol licensing standard

For LI.FI-owned Solidity sources under src/**, the documented standard is // SPDX-License-Identifier: LGPL-3.0-only with a blank line before pragma. TokenWrapper currently uses UNLICENSED. Unless there is a deliberate legal exception for this contract, it should be updated to the LGPL header to stay consistent with the rest of the codebase and the stated guidelines.

🧹 Nitpick comments (10)

script/deploy/zksync/DeployTokenWrapper.zksync.s.sol (2)

32-43: Converter handling and constructor arg ordering look correct

The optional converterAddress parsing with a try/catch and address(0) fallback is a clean way to keep existing deployments working while enabling the new parameter. Encoding constructor args as (wrappedNativeAddress, converterAddress, refundWalletAddress) matches the new TokenWrapper constructor signature and keeps ordering consistent across scripts.

You could slightly reduce I/O by reading path once into a local string and reusing it for both wrappedNativeAddress and converterAddress, but that’s purely cosmetic here.

Also applies to: 59-64

1-2: Align deployment script SPDX header and spacing with repo guidelines

Per script/**/*.sol guidelines, deployment scripts should use // SPDX-License-Identifier: LGPL-3.0-only and have a blank line before the pragma statement. This file currently uses UNLICENSED and no blank line; consider updating to match the standard.

script/deploy/facets/DeployTokenWrapper.s.sol (2)

32-43: Facet deploy constructor args are consistent with core contract and other scripts

The converter lookup and fallback to address(0) match the zksync deploy script, and the encoded args (wrappedNativeAddress, converterAddress, refundWalletAddress) align with TokenWrapper’s constructor ordering. That keeps deployments coherent across environments.

Given the zksync and facet scripts now share almost identical getConstructorArgs logic, you might consider extracting a small helper in DeployScriptBase or a shared util to avoid future drift, but it’s not strictly necessary.

Also applies to: 59-64

1-2: Update SPDX header and spacing to match src/**/*.sol / script/**/*.sol conventions

As with the zksync script, this file uses UNLICENSED and no blank line before pragma. The repo guidelines call for LGPL-3.0-only and a blank line before the pragma in Solidity scripts. Updating this keeps deployment scripts consistent with the documented standard.

script/deploy/tron/deploy-and-register-periphery.ts (1)

805-815: TokenWrapper constructor args and converter handling on Tron are consistent, with minor DRY opportunity

The new converterHex derivation and expanded constructorArgs [wrappedNativeHex, converterHex, refundWalletHex] correctly mirror the updated TokenWrapper constructor and keep behavior backward-compatible when tronConfig.converterAddress is unset (zero address). The additional logging around the converter address is also useful operationally.

The only nit is duplication: the converterHex computation and the associated consola.info("Using converter: ...") block are repeated in both redeploy and fresh-deploy branches. You could compute converterHex once just before those branches and reuse it, which would make future changes easier. Also, please double-check that tronConfig.converterAddress in config/networks.json is always in the format TronWeb.address.toHex expects (e.g., base58 Tron address), otherwise the hex conversion here could misbehave.

Also applies to: 820-826, 891-901, 906-912

test/solidity/Periphery/TokenWrapper.t.sol (2)

19-23: Constructor usage matches updated TokenWrapper signature

The updated instantiation new TokenWrapper(address(wrappedToken), address(0), address(this)) correctly matches the new constructor parameters (_wrappedToken, _converter, _owner) and keeps existing tests exercising the “no converter configured” path.

If you plan to rely on non-zero converters in production, it could be valuable to add a small harness test with a dummy IWrapper-compatible converter to cover that branch end-to-end (deposit + withdraw), but the current change itself is sound.

1-2: Test file SPDX header differs from documented test/**/*.sol convention

Guidelines call for Solidity test files to use // SPDX-License-Identifier: LGPL-3.0-only and a blank line before pragma. This test currently uses Unlicense. Consider aligning it with the project’s standard unless this file is intentionally kept under a different license for external reasons.

src/Periphery/TokenWrapper.sol (3)

21-25: Constructor extension and new converter state are consistent, but rely on deployment-time correctness

The added converter state and extended constructor constructor(address _wrappedToken, address _converter, address _owner) match how all deployment scripts and tests now encode arguments (wrappedNative, converter, refundWallet/owner), so the wiring looks consistent.

You’re intentionally allowing _converter to be address(0) as a sentinel for “no converter”, which deposit/withdraw respect. That’s fine, but note that there is still no validation on _wrappedToken or _owner; a misconfigured deployment could leave the contract unusable. Given prior patterns in this repo (constructor arg validation handled in scripts rather than on-chain), this is probably acceptable, but worth double-checking that all new networks set a sensible _wrappedToken and _owner in config.

Also applies to: 33-39

45-49: Converter-based wrap/unwrap flow is coherent; ensure configured converters truly match the expected interface

The updated deposit/withdraw logic correctly:

Selects wrapperAddress = converter != address(0) ? converter : wrappedToken so legacy deployments (no converter) keep using the wrapped token directly.

On withdraw, transfers the caller’s entire wrappedToken balance into this contract, optionally sets an allowance for converter via LibAsset.maxApproveERC20, and then calls IWrapper(wrapperAddress).withdraw(wad) before forwarding the received native to msg.sender via SafeTransferLib.safeTransferETH.

Adds receive() so the contract can accept native tokens from wrappedToken or any configured converter.

Functionally this is sound and preserves the previous behavior when converter == address(0). The main assumption is that any non-zero converter you wire in:

Implements the exact IWrapper interface (deposit() payable, withdraw(uint wad)), and

Mints/burns the same wrappedToken the contract tracks, in such a way that TokenWrapper can hold/burn those tokens (i.e., deposit credits TokenWrapper, and withdraw consumes tokens from TokenWrapper using the allowance you set).

If a converter deviates from that pattern, these functions could revert or misbehave. I recommend explicitly verifying the deployed converter contracts against this expectation and, if possible, documenting that invariant near the converter state variable or in the configuration docs.

Also, LibAsset.maxApproveERC20 will (by design) leave a large allowance to the converter; that matches existing patterns in this repo, but it reinforces the requirement that converter be treated as a trusted component.

Also applies to: 60-71, 73-74

23-28: Minor clean-up: unused constant and error

MAX_INT and WithdrawFailure don’t appear to be used anymore. They can be safely removed to reduce clutter unless they’re kept for backward compatibility or near-term planned usage.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 5b6c862 and 6c63ef9.

📒 Files selected for processing (5)

script/deploy/facets/DeployTokenWrapper.s.sol (2 hunks)
script/deploy/tron/deploy-and-register-periphery.ts (2 hunks)
script/deploy/zksync/DeployTokenWrapper.zksync.s.sol (2 hunks)
src/Periphery/TokenWrapper.sol (3 hunks)
test/solidity/Periphery/TokenWrapper.t.sol (1 hunks)

🧰 Additional context used

📓 Path-based instructions (6)

script/**/*.sol

📄 CodeRabbit inference engine (conventions.md)

All Solidity deployment and task script files must start with SPDX-License-Identifier comment (LGPL-3.0-only), followed by a blank line, then pragma statement

Files:

script/deploy/zksync/DeployTokenWrapper.zksync.s.sol
script/deploy/facets/DeployTokenWrapper.s.sol

script/deploy/facets/*.s.sol

📄 CodeRabbit inference engine (conventions.md)

script/deploy/facets/*.s.sol: Facet deployment scripts must inherit DeployScriptBase, call deploy() with type(FacetName).creationCode. For facets without constructor args, no getConstructorArgs() function needed. For facets with constructor args, implement getConstructorArgs() returning encoded abi.encode(arg1, arg2)
Facet update scripts must inherit UpdateScriptBase, call update("{ContractName}"), and use getExcludes() for special cases returning array of excluded function selectors

Files:

script/deploy/facets/DeployTokenWrapper.s.sol

test/**/*.sol

📄 CodeRabbit inference engine (conventions.md)

All Solidity test files must start with SPDX-License-Identifier comment (LGPL-3.0-only), followed by a blank line, then pragma statement

Files:

test/solidity/Periphery/TokenWrapper.t.sol

test/**/*.t.sol

📄 CodeRabbit inference engine (conventions.md)

test/**/*.t.sol: Test files must have .t.sol extension. All successful tests must be prefixed with test_, failure/revert tests prefixed with testRevert_, and inherited tests prefixed with testBase_. Every test contract must have setUp() function that calls initTestBase() if inheriting from TestBase
In test files, use blank lines between vm.expectRevert() and function call, before vm.stopPrank() if a separate logical block, and before assertions. Single blank line between test cases, after vm.startPrank(address), and before/after vm.expectEmit blocks (no blank line between vm.expectEmit and its event definition)
Use vm.expectRevert() to verify specific revert reasons in failure test cases (simply checking success/failure status of call() is not sufficient). Use vm.expectEmit(true, true, true, true, ) for event testing
For facets requiring whitelist functionality, test contracts should inherit from TestWhitelistManagerBase and use its functions (addToWhitelist, removeFromWhitelist, setFunctionWhitelistBySelector, removeFunctionApprovalBySelector)

Files:

test/solidity/Periphery/TokenWrapper.t.sol

src/**/*.sol

📄 CodeRabbit inference engine (conventions.md)

src/**/*.sol: All Solidity files must start with SPDX-License-Identifier comment (LGPL-3.0-only for LI.FI code), followed by a blank line, then pragma statement
Use LGPL-3.0-only license identifier for all LI.FI Solidity files, with exceptions for external dependencies and copied contracts which must retain original license and note modifications
Error names must be descriptive, follow PascalCase, and not include error messages (for gas optimization). Use custom error types rather than generic revert() statements
Use camelCase for state variables (e.g., userBalance, tokenAddress), camelCase with underscore prefix for function parameters (e.g., _amount, _recipient), CONSTANT_CASE for constants and immutable variables (e.g., MAX_FEE, RELAY_DEPOSITORY), and camelCase for function names (e.g., startBridge, validateInput)
Every contract must have NatSpec documentation with @title, @author (always 'LI.FI (https://li.fi)'), @notice, and @Custom:version (semantic versioning X.Y.Z) tags in that specific order
Every public/external function must have NatSpec documentation including description, @param for each parameter, and @return for return values. Note if function is restricted to admin or specific addresses
Include single blank lines between logical sections (state variables, events, constructor, functions), between function declarations, before new logical blocks inside functions, and before return statements. No blank lines between function signature and body or between if statements and their revert() calls. No blank line between related consecutive mappings

Files:

src/Periphery/TokenWrapper.sol

script/**/*.ts

📄 CodeRabbit inference engine (conventions.md)

script/**/*.ts: Do NOT use deprecated ethers-based helpers: getProvider(), getWalletFromPrivateKeyInDotEnv(), deprecated sendTransaction(), or ensureBalanceAndAllowanceToDiamond(). Use viem clients and viem-based helpers instead
All TypeScript scripts must follow rules defined in .eslintrc.cjs, use async/await for asynchronous operations, handle errors with try/catch blocks, use citty for CLI argument parsing, consola for logging, getEnvVar() for environment variables, and exit with appropriate codes (0 for success, 1 for error)

Files:

script/deploy/tron/deploy-and-register-periphery.ts

🧠 Learnings (30)

📓 Common learnings

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 946
File: config/networks.json:260-260
Timestamp: 2025-01-23T02:26:29.900Z
Learning: Each network in the configuration has its own unique wrapped native token address specified in the `wrappedNativeAddress` field. These addresses are network-specific and should not be expected to be the same or available across different networks.

Learnt from: ezynda3
Repo: lifinance/contracts PR: 984
File: src/Periphery/ReceiverChainflip.sol:48-62
Timestamp: 2025-02-24T09:35:34.908Z
Learning: Contract address validations for periphery contracts like ReceiverChainflip are handled in deployment scripts rather than in the constructor.

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1140
File: deployments/fantom.diamond.json:97-105
Timestamp: 2025-06-25T06:27:38.873Z
Learning: When contracts are redeployed, they receive new addresses. Permit2Proxy addresses in deployment files reflect the actual deployed contract addresses for each network, not a standardized address across all networks.

Learnt from: ezynda3
Repo: lifinance/contracts PR: 1124
File: script/demoScripts/utils/lib/cowShedSdk.ts:0-0
Timestamp: 2025-06-05T14:50:17.275Z
Learning: For demo scripts and example code, ezynda3 prefers to keep the code simple and focused on demonstrating the core functionality rather than adding extensive input validation or defensive programming measures.

📚 Learning: 2024-11-25T13:49:40.464Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 782
File: script/deploy/zksync/DeployPermit2Proxy.s.sol:22-61
Timestamp: 2024-11-25T13:49:40.464Z
Learning: In the deploy scripts (e.g., `script/deploy/zksync/DeployPermit2Proxy.s.sol`), complex error handling and address validation are not necessary. The team prefers to keep deploy scripts simple without extensive error handling.

Applied to files:

script/deploy/zksync/DeployTokenWrapper.zksync.s.sol
script/deploy/facets/DeployTokenWrapper.s.sol
script/deploy/tron/deploy-and-register-periphery.ts

📚 Learning: 2024-11-25T13:50:34.522Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 782
File: script/deploy/zksync/DeployPermit2Proxy.s.sol:13-20
Timestamp: 2024-11-25T13:50:34.522Z
Learning: Contracts on ZkSync will always have different addresses compared to other EVM networks. Do not highlight address differences for ZkSync when verifying deployment configurations or in log files.

Applied to files:

script/deploy/zksync/DeployTokenWrapper.zksync.s.sol

📚 Learning: 2025-01-30T08:16:28.174Z

Learnt from: ezynda3
Repo: lifinance/contracts PR: 924
File: script/deploy/zksync/DeployExecutor.zksync.s.sol:40-46
Timestamp: 2025-01-30T08:16:28.174Z
Learning: Validation for withdraw wallet address is not required in DeployExecutor.zksync.s.sol as it's handled elsewhere in the system.

Applied to files:

script/deploy/zksync/DeployTokenWrapper.zksync.s.sol

📚 Learning: 2025-01-23T02:26:29.900Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 946
File: config/networks.json:260-260
Timestamp: 2025-01-23T02:26:29.900Z
Learning: Each network in the configuration has its own unique wrapped native token address specified in the `wrappedNativeAddress` field. These addresses are network-specific and should not be expected to be the same or available across different networks.

Applied to files:

script/deploy/zksync/DeployTokenWrapper.zksync.s.sol
script/deploy/facets/DeployTokenWrapper.s.sol

📚 Learning: 2025-04-21T03:17:53.443Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1109
File: deployments/worldchain.json:28-28
Timestamp: 2025-04-21T03:17:53.443Z
Learning: For deployment PRs updating contract addresses (like RelayFacet on Worldchain), verify the presence of entries in all relevant files (worldchain.json, worldchain.diamond.json, _deployments_log_file.json) before reporting inconsistencies. The RelayFacet entry exists in all required deployment files with the correct address.

Applied to files:

script/deploy/zksync/DeployTokenWrapper.zksync.s.sol
script/deploy/facets/DeployTokenWrapper.s.sol
script/deploy/tron/deploy-and-register-periphery.ts

📚 Learning: 2025-06-25T06:27:38.873Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1140
File: deployments/fantom.diamond.json:97-105
Timestamp: 2025-06-25T06:27:38.873Z
Learning: When contracts are redeployed, they receive new addresses. Permit2Proxy addresses in deployment files reflect the actual deployed contract addresses for each network, not a standardized address across all networks.

Applied to files:

script/deploy/zksync/DeployTokenWrapper.zksync.s.sol
script/deploy/facets/DeployTokenWrapper.s.sol
script/deploy/tron/deploy-and-register-periphery.ts

📚 Learning: 2024-10-04T09:17:19.275Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 819
File: deployments/avalanche.diamond.json:105-105
Timestamp: 2024-10-04T09:17:19.275Z
Learning: In the `lifinance/contracts` repository, contracts may have different addresses across networks. Do not check if contracts have the same addresses across all networks in future reviews.

Applied to files:

script/deploy/zksync/DeployTokenWrapper.zksync.s.sol

📚 Learning: 2024-11-25T09:05:43.045Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 782
File: deployments/base.diamond.json:148-148
Timestamp: 2024-11-25T09:05:43.045Z
Learning: In deployment configuration files (e.g., `deployments/base.diamond.json`), empty addresses for contracts like `Permit2Proxy` may be placeholders and will be updated after approvals are released from the multisig safe.

Applied to files:

script/deploy/zksync/DeployTokenWrapper.zksync.s.sol
script/deploy/facets/DeployTokenWrapper.s.sol
script/deploy/tron/deploy-and-register-periphery.ts

📚 Learning: 2025-01-09T04:34:00.778Z

Learnt from: ezynda3
Repo: lifinance/contracts PR: 909
File: script/deploy/zksync/DeployTokenWrapper.s.sol:43-49
Timestamp: 2025-01-09T04:34:00.778Z
Learning: In deployment scripts for LiFi contracts, validation for refundWallet address is not required as it's handled elsewhere or through configuration validation.

Applied to files:

script/deploy/zksync/DeployTokenWrapper.zksync.s.sol

📚 Learning: 2025-09-25T00:12:58.536Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1395
File: src/Periphery/FeeForwarder.sol:78-120
Timestamp: 2025-09-25T00:12:58.536Z
Learning: In src/Periphery/FeeForwarder.sol, the team intentionally uses address(this).balance for refunding remaining native tokens because the contract is designed to never hold funds and not collect dust, making it safe to return the entire balance to the caller.

Applied to files:

script/deploy/zksync/DeployTokenWrapper.zksync.s.sol
script/deploy/facets/DeployTokenWrapper.s.sol
src/Periphery/TokenWrapper.sol

📚 Learning: 2025-10-13T11:13:48.847Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 1413
File: src/Facets/EverclearV2Facet.sol:75-96
Timestamp: 2025-10-13T11:13:48.847Z
Learning: In EverclearV2Facet (and all LiFi facets), the team standard is to use msg.sender as the refund address with the refundExcessNative modifier, not requiring an explicit refund address parameter.

Applied to files:

script/deploy/zksync/DeployTokenWrapper.zksync.s.sol
script/deploy/facets/DeployTokenWrapper.s.sol

📚 Learning: 2025-12-02T10:08:06.478Z

Learnt from: CR
Repo: lifinance/contracts PR: 0
File: conventions.md:0-0
Timestamp: 2025-12-02T10:08:06.478Z
Learning: Applies to src/Facets/*.sol : For non-EVM chain support in facets, use bytes (not bytes32) for receiverAddress, validate non-EVM address is not zero, and use NON_EVM_ADDRESS constant from src/Helpers/LiFiData.sol when needed

Applied to files:

script/deploy/facets/DeployTokenWrapper.s.sol

📚 Learning: 2024-10-31T09:07:36.301Z

Learnt from: ezynda3
Repo: lifinance/contracts PR: 843
File: script/deploy/facets/DeployRelayFacet.s.sol:0-0
Timestamp: 2024-10-31T09:07:36.301Z
Learning: In `script/deploy/facets/DeployRelayFacet.s.sol`, additional validation for the configuration file existence and address validity is unnecessary because the code will already fail appropriately if the configuration file is missing or the addresses are invalid.

Applied to files:

script/deploy/facets/DeployTokenWrapper.s.sol

📚 Learning: 2025-09-09T10:39:26.383Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1357
File: deployments/lens.diamond.json:48-51
Timestamp: 2025-09-09T10:39:26.383Z
Learning: In the lifinance/contracts repository, when deployment JSON files show address changes (like AcrossFacetV3 address updates in deployments/*.diamond.json), the corresponding _deployments_log_file.json updates may be handled in separate PRs rather than the same PR that updates the diamond configuration files.

Applied to files:

script/deploy/facets/DeployTokenWrapper.s.sol

📚 Learning: 2025-07-03T07:34:47.349Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1184
File: script/deploy/safe/deploy-safe.ts:0-0
Timestamp: 2025-07-03T07:34:47.349Z
Learning: In `script/deploy/safe/deploy-safe.ts`, 0xDEnYO intentionally changed the `allowOverride` flag default from `false` to `true` to allow overwriting existing Safe addresses by default in deployment workflows.

Applied to files:

script/deploy/facets/DeployTokenWrapper.s.sol
script/deploy/tron/deploy-and-register-periphery.ts

📚 Learning: 2025-04-21T03:17:53.443Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1109
File: deployments/worldchain.json:28-28
Timestamp: 2025-04-21T03:17:53.443Z
Learning: For deployment PRs involving address updates like the RelayFacet to Worldchain, verify the actual presence of entries in files before reporting issues. The RelayFacet exists in the diamond log file and the PR diff already contains the necessary address change.

Applied to files:

script/deploy/facets/DeployTokenWrapper.s.sol

📚 Learning: 2025-08-27T22:23:51.257Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: src/Periphery/Lda/Facets/NativeWrapperFacet.sol:0-0
Timestamp: 2025-08-27T22:23:51.257Z
Learning: In src/Periphery/Lda/Facets/NativeWrapperFacet.sol, the wrapNative function should not validate msg.value == amountIn because CoreRouteFacet's DistributeNative command (command type 3) overrides the amountIn parameter with address(this).balance, making such validation incorrect and causing false failures.

Applied to files:

script/deploy/facets/DeployTokenWrapper.s.sol
test/solidity/Periphery/TokenWrapper.t.sol

📚 Learning: 2025-08-29T11:12:02.184Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: src/Periphery/LDA/Facets/SyncSwapV2Facet.sol:33-38
Timestamp: 2025-08-29T11:12:02.184Z
Learning: In LDA facets (src/Periphery/Lda/Facets/**/*.sol), tokenIn parameter validation (checking for address(0)) is intentionally not performed as part of the design architecture.

Applied to files:

test/solidity/Periphery/TokenWrapper.t.sol

📚 Learning: 2025-12-02T10:08:06.478Z

Learnt from: CR
Repo: lifinance/contracts PR: 0
File: conventions.md:0-0
Timestamp: 2025-12-02T10:08:06.478Z
Learning: Applies to test/**/*.t.sol : For facets requiring whitelist functionality, test contracts should inherit from TestWhitelistManagerBase and use its functions (addToWhitelist, removeFromWhitelist, setFunctionWhitelistBySelector, removeFunctionApprovalBySelector)

Applied to files:

test/solidity/Periphery/TokenWrapper.t.sol

📚 Learning: 2025-05-06T09:09:38.108Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 1117
File: test/solidity/Periphery/LiFiDEXAggregator.t.sol:111-127
Timestamp: 2025-05-06T09:09:38.108Z
Learning: The `setupApechain()` function in LiFiDEXAggregator tests intentionally avoids calling `initTestBase()` to prevent needing to define standard contracts (like ADDRESS_USDC_PROXY and Uniswap) that aren't needed for Apechain-specific tests.

Applied to files:

test/solidity/Periphery/TokenWrapper.t.sol

📚 Learning: 2024-10-14T08:23:38.076Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 831
File: src/Helpers/WithdrawablePeriphery.sol:26-30
Timestamp: 2024-10-14T08:23:38.076Z
Learning: In the `WithdrawablePeriphery.sol` contract, when transferring Ether in the `withdrawToken` function, prefer using `SafeTransferLib.safeTransferETH` instead of low-level `call` for safer Ether transfers.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-01-28T14:26:29.054Z

Learnt from: ezynda3
Repo: lifinance/contracts PR: 924
File: script/deploy/zksync/DeployExecutor.s.sol:48-48
Timestamp: 2025-01-28T14:26:29.054Z
Learning: The Executor contract constructor accepts two parameters:
1. _erc20Proxy: The address of the ERC20Proxy contract
2. _owner: The owner address (passed to WithdrawablePeriphery)

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-08-20T09:56:24.259Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1327
File: docs/OutputValidator.md:23-29
Timestamp: 2025-08-20T09:56:24.259Z
Learning: In src/Periphery/OutputValidator.sol, the ERC20 token flow that uses balanceOf(msg.sender) to compute actualAmount and transfers the excess via safeTransferFrom is intentional behavior, as confirmed by 0xDEnYO. The design choice to potentially include pre-existing ERC20 balances in the excess calculation is by design and should not be flagged as a security concern in future reviews.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-08-27T23:36:40.773Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: test/solidity/Periphery/GasZipPeriphery.t.sol:223-233
Timestamp: 2025-08-27T23:36:40.773Z
Learning: In bridge facet swap flows with requiresDeposit=false, tokens are already held by the diamond contract from previous swaps. When subsequent contracts like GasZipPeriphery call LibAsset.depositAsset, they pull tokens from msg.sender (the diamond) to themselves via transferFrom, requiring no additional approvals since the diamond already owns the tokens.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-04-28T07:46:24.084Z

Learnt from: ezynda3
Repo: lifinance/contracts PR: 1116
File: src/Libraries/LibAsset.sol:102-108
Timestamp: 2025-04-28T07:46:24.084Z
Learning: In the LiFi contracts, the `LibAsset.transferFromERC20` function is intentionally designed to accept arbitrary `from` parameters, with security ensured through a whitelist mechanism that controls which contracts can call this function.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-02-24T09:35:34.908Z

Learnt from: ezynda3
Repo: lifinance/contracts PR: 984
File: src/Periphery/ReceiverChainflip.sol:48-62
Timestamp: 2025-02-24T09:35:34.908Z
Learning: Contract address validations for periphery contracts like ReceiverChainflip are handled in deployment scripts rather than in the constructor.

Applied to files:

script/deploy/tron/deploy-and-register-periphery.ts

📚 Learning: 2025-04-22T09:04:44.244Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1112
File: deployments/soneium.diamond.json:81-81
Timestamp: 2025-04-22T09:04:44.244Z
Learning: In the lifinance/contracts repository, it's normal and expected for periphery contracts to have empty address values in deployment files when they are not deployed on a particular network. This applies to all periphery contracts including ReceiverChainflip, ReceiverStargateV2, and others. PRs should not flag empty periphery contract addresses as issues.

Applied to files:

script/deploy/tron/deploy-and-register-periphery.ts

📚 Learning: 2025-04-21T03:15:12.236Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1109
File: deployments/worldchain.diamond.json:84-85
Timestamp: 2025-04-21T03:15:12.236Z
Learning: In deployment JSON files that contain "diamond" in their filename (located in the deployments folder), periphery contracts may have empty string values for their addresses. This indicates that the contract is not deployed on that particular chain and should not be flagged as an issue during code reviews.

Applied to files:

script/deploy/tron/deploy-and-register-periphery.ts

📚 Learning: 2025-01-21T11:07:36.236Z

Learnt from: ezynda3
Repo: lifinance/contracts PR: 937
File: script/deploy/facets/DeployLiFiDEXAggregator.s.sol:33-35
Timestamp: 2025-01-21T11:07:36.236Z
Learning: In the LiFiDEXAggregator contract deployment (DeployLiFiDEXAggregator.s.sol), the refundWallet address from global.json is used as the owner parameter in the contract's constructor, which is then passed to the WithdrawablePeriphery base contract.

Applied to files:

script/deploy/tron/deploy-and-register-periphery.ts

🪛 GitHub Check: Olympix Integrated Security

src/Periphery/TokenWrapper.sol

[notice] 34-34:
Parameters passed to a constructor that are not validated for correct values may lead to contract creation in an undesired state. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/no-parameter-validation-in-constructor

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)

GitHub Check: enforce-min-test-coverage
GitHub Check: run-unit-tests

… converter support

coderabbitai

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/Periphery/TokenWrapper.sol (1)
1-2: Update SPDX identifier for periphery contract

Per the coding guidelines, LI.FI Solidity contracts under src/ should use LGPL-3.0-only as the license identifier. This file currently uses UNLICENSED.

Suggested change:
-// SPDX-License-Identifier: UNLICENSED
+// SPDX-License-Identifier: LGPL-3.0-only

🧹 Nitpick comments (4)

test/solidity/utils/TestBasicToken.sol (1)

1-22: Align SPDX license with repo conventions

Implementation looks fine for a test helper (simple ERC20 with unrestricted mint/burn is appropriate in this context). However, the SPDX header currently uses Unlicense, while the conventions call for LGPL-3.0-only on LI.FI-authored Solidity files.

Consider updating the header:
-// SPDX-License-Identifier: Unlicense
+// SPDX-License-Identifier: LGPL-3.0-only

test/solidity/utils/TestConverterWithDecimals.sol (1)

1-56: Decimal converter helper is sound; consider minor cleanups

The 18↔6 decimal conversion and balance flows look correct for the intended test usage, and they integrate cleanly with TokenWrapper’s balance-delta accounting.

Two small optional cleanups:

error InvalidAmount(); is currently unused; either remove it or switch the require in deposit() to use the custom error, e.g.:
```
if (msg.value % 1e12 != 0) {
    revert InvalidAmount();
}
```
To keep SPDX identifiers consistent with the rest of the repo, you may want to switch to LGPL-3.0-only here as well:

-// SPDX-License-Identifier: Unlicense
+// SPDX-License-Identifier: LGPL-3.0-only


</blockquote></details>
<details>
<summary>src/Periphery/TokenWrapper.sol (1)</summary><blockquote>

`21-43`: **Converter integration and balance‑delta accounting look correct; a couple of small nits**

The new converter wiring (`converter`, `wrapperAddress`, `useConverter`) and the updated `deposit`/`withdraw` flows look sound:

- With `useConverter == false`, behavior matches the previous WETH9-style wrapper: deposit forwards ETH to the wrapped token and transfers `msg.value` WETH, withdraw pulls the full token balance and unwraps 1:1.
- With `useConverter == true`, using balance deltas (`IERC20(wrappedToken).balanceOf` and `address(this).balance`) to compute `amountReceived` is a good way to make the wrapper agnostic to converter-specific decimals and fees and will revert on underflow if the converter misbehaves.
- The approval pattern `LibAsset.maxApproveERC20(IERC20(wrappedToken), converter, wad)` is appropriate to let the converter pull tokens from the wrapper without repeated approvals. Constructor-level parameter validation isn’t needed here because deployRequirements.json enforces correct deployment config before these scripts run. Based on learnings, this matches existing repo practice.

Minor cleanups you might consider:

- `error WithdrawFailure();` is no longer used; removing it would slightly reduce bytecode size.
- `IERC20(wrappedToken).approve(address(this), MAX_INT);` in the constructor sets an allowance from the deployer to this contract, which isn’t used anywhere in the current logic (all flows rely on user → wrapper approvals and wrapper → converter approvals). It can likely be dropped.




Also applies to: 48-87

</blockquote></details>
<details>
<summary>test/solidity/utils/TestWrappedConverter.sol (1)</summary><blockquote>

`1-44`: **WETH9‑style test converter looks correct; optional tidy‑ups**

The mock converter behavior (deposit sending wrapped tokens equal to `msg.value`, withdraw pulling tokens via `transferFrom` and sending back ETH) matches how `TokenWrapper` uses a WETH9-compatible interface, so it’s suitable for testing.

Optional cleanups:

- `error InsufficientBalance(uint256 available, uint256 required);` is unused; you could remove it or use it in `withdraw` if you later add explicit balance checks.
- For consistency with other LI.FI Solidity, consider switching the SPDX to `LGPL-3.0-only` if these test helpers are meant to follow the same licensing:

```diff
-// SPDX-License-Identifier: Unlicense
+// SPDX-License-Identifier: LGPL-3.0-only

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 6c63ef9 and 633c97f.

📒 Files selected for processing (5)

src/Periphery/TokenWrapper.sol (3 hunks)
test/solidity/Periphery/TokenWrapper.t.sol (2 hunks)
test/solidity/utils/TestBasicToken.sol (1 hunks)
test/solidity/utils/TestConverterWithDecimals.sol (1 hunks)
test/solidity/utils/TestWrappedConverter.sol (1 hunks)

🧰 Additional context used

📓 Path-based instructions (3)

test/**/*.sol

📄 CodeRabbit inference engine (conventions.md)

All Solidity test files must start with SPDX-License-Identifier comment (LGPL-3.0-only), followed by a blank line, then pragma statement

Files:

test/solidity/utils/TestWrappedConverter.sol
test/solidity/utils/TestConverterWithDecimals.sol
test/solidity/Periphery/TokenWrapper.t.sol
test/solidity/utils/TestBasicToken.sol

test/**/*.t.sol

📄 CodeRabbit inference engine (conventions.md)

test/**/*.t.sol: Test files must have .t.sol extension. All successful tests must be prefixed with test_, failure/revert tests prefixed with testRevert_, and inherited tests prefixed with testBase_. Every test contract must have setUp() function that calls initTestBase() if inheriting from TestBase
In test files, use blank lines between vm.expectRevert() and function call, before vm.stopPrank() if a separate logical block, and before assertions. Single blank line between test cases, after vm.startPrank(address), and before/after vm.expectEmit blocks (no blank line between vm.expectEmit and its event definition)
Use vm.expectRevert() to verify specific revert reasons in failure test cases (simply checking success/failure status of call() is not sufficient). Use vm.expectEmit(true, true, true, true, ) for event testing
For facets requiring whitelist functionality, test contracts should inherit from TestWhitelistManagerBase and use its functions (addToWhitelist, removeFromWhitelist, setFunctionWhitelistBySelector, removeFunctionApprovalBySelector)

Files:

test/solidity/Periphery/TokenWrapper.t.sol

src/**/*.sol

📄 CodeRabbit inference engine (conventions.md)

src/**/*.sol: All Solidity files must start with SPDX-License-Identifier comment (LGPL-3.0-only for LI.FI code), followed by a blank line, then pragma statement
Use LGPL-3.0-only license identifier for all LI.FI Solidity files, with exceptions for external dependencies and copied contracts which must retain original license and note modifications
Error names must be descriptive, follow PascalCase, and not include error messages (for gas optimization). Use custom error types rather than generic revert() statements
Use camelCase for state variables (e.g., userBalance, tokenAddress), camelCase with underscore prefix for function parameters (e.g., _amount, _recipient), CONSTANT_CASE for constants and immutable variables (e.g., MAX_FEE, RELAY_DEPOSITORY), and camelCase for function names (e.g., startBridge, validateInput)
Every contract must have NatSpec documentation with @title, @author (always 'LI.FI (https://li.fi)'), @notice, and @Custom:version (semantic versioning X.Y.Z) tags in that specific order
Every public/external function must have NatSpec documentation including description, @param for each parameter, and @return for return values. Note if function is restricted to admin or specific addresses
Include single blank lines between logical sections (state variables, events, constructor, functions), between function declarations, before new logical blocks inside functions, and before return statements. No blank lines between function signature and body or between if statements and their revert() calls. No blank line between related consecutive mappings

Files:

src/Periphery/TokenWrapper.sol

🧠 Learnings (30)

📓 Common learnings

Learnt from: ezynda3
Repo: lifinance/contracts PR: 1124
File: script/demoScripts/utils/lib/cowShedSdk.ts:0-0
Timestamp: 2025-06-05T14:50:17.275Z
Learning: For demo scripts and example code, ezynda3 prefers to keep the code simple and focused on demonstrating the core functionality rather than adding extensive input validation or defensive programming measures.

📚 Learning: 2024-10-10T03:18:20.721Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 782
File: test/solidity/Helpers/WithdrawablePeriphery.t.sol:22-35
Timestamp: 2024-10-10T03:18:20.721Z
Learning: In Solidity tests for withdrawal functions in `test/solidity/Helpers/WithdrawablePeriphery.t.sol`, do not suggest adding tests where the withdrawal amount exceeds the contract's balance, as such tests are unnecessary because any contract will fail in that case.

Applied to files:

test/solidity/utils/TestWrappedConverter.sol
test/solidity/utils/TestConverterWithDecimals.sol
test/solidity/Periphery/TokenWrapper.t.sol
test/solidity/utils/TestBasicToken.sol

📚 Learning: 2025-12-02T10:08:06.478Z

Learnt from: CR
Repo: lifinance/contracts PR: 0
File: conventions.md:0-0
Timestamp: 2025-12-02T10:08:06.478Z
Learning: Applies to test/**/*.t.sol : For facets requiring whitelist functionality, test contracts should inherit from TestWhitelistManagerBase and use its functions (addToWhitelist, removeFromWhitelist, setFunctionWhitelistBySelector, removeFunctionApprovalBySelector)

Applied to files:

test/solidity/utils/TestWrappedConverter.sol
test/solidity/Periphery/TokenWrapper.t.sol
test/solidity/utils/TestBasicToken.sol

📚 Learning: 2024-10-14T08:23:38.076Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 831
File: src/Helpers/WithdrawablePeriphery.sol:26-30
Timestamp: 2024-10-14T08:23:38.076Z
Learning: In the `WithdrawablePeriphery.sol` contract, when transferring Ether in the `withdrawToken` function, prefer using `SafeTransferLib.safeTransferETH` instead of low-level `call` for safer Ether transfers.

Applied to files:

test/solidity/utils/TestWrappedConverter.sol
test/solidity/utils/TestConverterWithDecimals.sol
test/solidity/Periphery/TokenWrapper.t.sol
src/Periphery/TokenWrapper.sol

📚 Learning: 2025-08-29T11:53:38.549Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: test/solidity/Periphery/LDA/BaseCoreRouteTest.t.sol:379-388
Timestamp: 2025-08-29T11:53:38.549Z
Learning: In test/solidity/Periphery/LDA/BaseCoreRouteTest.t.sol, for the revert-testing helper function _executeAndVerifySwap, only the aggregator branch (CommandType.DistributeSelfERC20) should use amountIn-1 to underfund and trigger insufficient balance errors, while user-funded branches should use the full amountIn to test other error conditions.

Applied to files:

test/solidity/utils/TestWrappedConverter.sol
test/solidity/utils/TestConverterWithDecimals.sol
test/solidity/Periphery/TokenWrapper.t.sol
test/solidity/utils/TestBasicToken.sol

📚 Learning: 2025-12-02T10:08:06.478Z

Learnt from: CR
Repo: lifinance/contracts PR: 0
File: conventions.md:0-0
Timestamp: 2025-12-02T10:08:06.478Z
Learning: Applies to test/**/*.t.sol : Test files must have .t.sol extension. All successful tests must be prefixed with test_, failure/revert tests prefixed with testRevert_, and inherited tests prefixed with testBase_. Every test contract must have setUp() function that calls initTestBase() if inheriting from TestBase

Applied to files:

test/solidity/utils/TestWrappedConverter.sol
test/solidity/Periphery/TokenWrapper.t.sol
test/solidity/utils/TestBasicToken.sol

📚 Learning: 2025-06-07T04:11:20.100Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1141
File: test/solidity/Periphery/LidoWrapper.t.sol:55-70
Timestamp: 2025-06-07T04:11:20.100Z
Learning: On Layer 2 networks, Lido's stETH contract has inverted function naming compared to expected behavior: `unwrap()` converts stETH → wstETH, and `wrap()` converts wstETH → stETH. This is the opposite of what the function names suggest but is how Lido implemented their L2 contracts.

Applied to files:

test/solidity/utils/TestWrappedConverter.sol

📚 Learning: 2025-01-22T12:38:37.557Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 945
File: test/solidity/utils/TestBase.sol:446-470
Timestamp: 2025-01-22T12:38:37.557Z
Learning: Test utility functions in TestBase.sol don't require production-level validations like slippage checks and input validation since they operate in a controlled test environment.

Applied to files:

test/solidity/utils/TestConverterWithDecimals.sol
test/solidity/Periphery/TokenWrapper.t.sol
test/solidity/utils/TestBasicToken.sol

📚 Learning: 2025-08-27T22:23:51.257Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: src/Periphery/Lda/Facets/NativeWrapperFacet.sol:0-0
Timestamp: 2025-08-27T22:23:51.257Z
Learning: In src/Periphery/Lda/Facets/NativeWrapperFacet.sol, the wrapNative function should not validate msg.value == amountIn because CoreRouteFacet's DistributeNative command (command type 3) overrides the amountIn parameter with address(this).balance, making such validation incorrect and causing false failures.

Applied to files:

test/solidity/Periphery/TokenWrapper.t.sol

📚 Learning: 2025-06-05T14:48:58.816Z

Learnt from: ezynda3
Repo: lifinance/contracts PR: 1124
File: src/Periphery/Patcher.sol:160-167
Timestamp: 2025-06-05T14:48:58.816Z
Learning: In src/Periphery/Patcher.sol, the unlimited token approval to finalTarget in depositAndExecuteWithDynamicPatches and depositAndExecuteWithMultiplePatches functions is intentional behavior, as confirmed by ezynda3. This design choice aligns with the contract's stateless/ephemeral architecture.

Applied to files:

test/solidity/Periphery/TokenWrapper.t.sol

📚 Learning: 2025-01-28T11:29:09.566Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 945
File: test/solidity/Facets/GlacisFacet.t.sol:0-0
Timestamp: 2025-01-28T11:29:09.566Z
Learning: Empty test methods with explanatory comments are acceptable in test classes when the feature being tested (e.g., native token bridging) is not supported by the implementation.

Applied to files:

test/solidity/Periphery/TokenWrapper.t.sol

📚 Learning: 2025-07-11T09:43:22.393Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 1193
File: test/solidity/Facets/AllowListMigratorFacet.t.sol:39-52
Timestamp: 2025-07-11T09:43:22.393Z
Learning: For fork-based tests like AllowListMigratorFacet.t.sol that work with existing deployed contracts (mainnet diamond), initTestBase() is intentionally omitted since the standard test initialization is not needed.

Applied to files:

test/solidity/Periphery/TokenWrapper.t.sol
test/solidity/utils/TestBasicToken.sol

📚 Learning: 2025-05-06T09:09:38.108Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 1117
File: test/solidity/Periphery/LiFiDEXAggregator.t.sol:111-127
Timestamp: 2025-05-06T09:09:38.108Z
Learning: The `setupApechain()` function in LiFiDEXAggregator tests intentionally avoids calling `initTestBase()` to prevent needing to define standard contracts (like ADDRESS_USDC_PROXY and Uniswap) that aren't needed for Apechain-specific tests.

Applied to files:

test/solidity/Periphery/TokenWrapper.t.sol

📚 Learning: 2025-01-22T12:36:12.699Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 945
File: test/solidity/Facets/GlacisFacet.t.sol:214-262
Timestamp: 2025-01-22T12:36:12.699Z
Learning: The GlacisFacet test suite inherits from TestBaseFacet which already covers various failure scenarios including invalid receiver address, invalid amounts, same chain bridging, and insufficient funds, making additional failure scenario tests redundant.

Applied to files:

test/solidity/Periphery/TokenWrapper.t.sol

📚 Learning: 2024-09-23T01:48:39.325Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 807
File: src/Periphery/GasZipPeriphery.sol:29-32
Timestamp: 2024-09-23T01:48:39.325Z
Learning: In our codebase, input validation for zero addresses in constructor parameters is not required because our deployment flow ensures that contracts cannot be deployed with zero address parameters unless we are okay with it.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-06-16T09:28:18.027Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1207
File: script/deploy/zksync/DeployPioneerFacet.zksync.s.sol:29-37
Timestamp: 2025-06-16T09:28:18.027Z
Learning: In the LIFI contracts codebase, constructor arguments in Solidity deployment scripts do not need validation because they are already validated by bash scripts in combination with the deployRequirements.json file. The deployRequirements.json file contains validation rules like "allowToDeployWithZeroAddress": false to prevent invalid constructor arguments during deployment.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-06-24T08:13:01.516Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1234
File: script/deploy/facets/DeployRelayFacet.s.sol:31-34
Timestamp: 2025-06-24T08:13:01.516Z
Learning: In the lifinance/contracts repository, parameter validation (like checking for address(0) or missing configuration values) should not be suggested in Foundry deploy scripts (files in script/deploy/) because the deployment process includes pre-validation through deployRequirements.json that checks these parameters before the Foundry scripts are executed. The deploy scripts can safely assume parameters are valid.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-02-11T10:33:52.791Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 988
File: script/deploy/facets/DeployPermit2Proxy.s.sol:33-37
Timestamp: 2025-02-11T10:33:52.791Z
Learning: The Permit2Proxy contract must not be deployed with zero addresses for its critical dependencies (LiFiDiamond, permit2Address, safeAddress). This is enforced by passing `false` to `_getConfigContractAddress` function.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-07-17T04:21:50.790Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1266
File: script/deploy/safe/execute-pending-timelock-tx.ts:129-129
Timestamp: 2025-07-17T04:21:50.790Z
Learning: In the lifinance/contracts repository, 0xDEnYO prefers not to add private key format validation in deployment scripts like execute-pending-timelock-tx.ts, prioritizing code simplicity over strict validation in their controlled environment.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-08-29T11:12:02.184Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: src/Periphery/LDA/Facets/SyncSwapV2Facet.sol:33-38
Timestamp: 2025-08-29T11:12:02.184Z
Learning: In LDA facets (src/Periphery/Lda/Facets/**/*.sol), tokenIn parameter validation (checking for address(0)) is intentionally not performed as part of the design architecture.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-07-16T01:03:08.106Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1275
File: src/Facets/AllBridgeFacet.sol:164-164
Timestamp: 2025-07-16T01:03:08.106Z
Learning: In src/Facets/AllBridgeFacet.sol, the team has decided that explicit validation for address downcasting from `_bridgeData.sendingAssetId` to `bytes32(uint256(uint160(_bridgeData.sendingAssetId)))` is not required, accepting the potential risk of silent overflow from unsafe downcasting.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-06-05T14:28:34.813Z

Learnt from: ezynda3
Repo: lifinance/contracts PR: 1124
File: script/demoScripts/demoPatcher.ts:0-0
Timestamp: 2025-06-05T14:28:34.813Z
Learning: For demo scripts in the lifinance/contracts repository, simpler validation (like checking for zero address) is acceptable and additional contract code validation is not necessary.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-07-17T04:21:55.549Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1266
File: script/deploy/safe/execute-pending-timelock-tx.ts:334-334
Timestamp: 2025-07-17T04:21:55.549Z
Learning: In the lifinance/contracts repository, 0xDEnYO prefers to keep private key processing simple in scripts like execute-pending-timelock-tx.ts without adding format validation, prioritizing code simplicity over strict validation in controlled environments.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-07-17T04:21:26.825Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1266
File: script/deploy/safe/execute-pending-timelock-tx.ts:627-628
Timestamp: 2025-07-17T04:21:26.825Z
Learning: In the lifinance/contracts repository, 0xDEnYO prefers to keep '0x0' as a fallback address in gas estimation calls rather than throwing errors when the wallet account address is not available, prioritizing code simplicity over strict validation.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-01-23T02:26:29.900Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 946
File: config/networks.json:260-260
Timestamp: 2025-01-23T02:26:29.900Z
Learning: Each network in the configuration has its own unique wrapped native token address specified in the `wrappedNativeAddress` field. These addresses are network-specific and should not be expected to be the same or available across different networks.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-01-28T14:26:29.054Z

Learnt from: ezynda3
Repo: lifinance/contracts PR: 924
File: script/deploy/zksync/DeployExecutor.s.sol:48-48
Timestamp: 2025-01-28T14:26:29.054Z
Learning: The Executor contract constructor accepts two parameters:
1. _erc20Proxy: The address of the ERC20Proxy contract
2. _owner: The owner address (passed to WithdrawablePeriphery)

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-09-25T00:12:58.536Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1395
File: src/Periphery/FeeForwarder.sol:78-120
Timestamp: 2025-09-25T00:12:58.536Z
Learning: In src/Periphery/FeeForwarder.sol, the team intentionally uses address(this).balance for refunding remaining native tokens because the contract is designed to never hold funds and not collect dust, making it safe to return the entire balance to the caller.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-08-20T09:56:24.259Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1327
File: docs/OutputValidator.md:23-29
Timestamp: 2025-08-20T09:56:24.259Z
Learning: In src/Periphery/OutputValidator.sol, the ERC20 token flow that uses balanceOf(msg.sender) to compute actualAmount and transfers the excess via safeTransferFrom is intentional behavior, as confirmed by 0xDEnYO. The design choice to potentially include pre-existing ERC20 balances in the excess calculation is by design and should not be flagged as a security concern in future reviews.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-04-28T07:46:24.084Z

Learnt from: ezynda3
Repo: lifinance/contracts PR: 1116
File: src/Libraries/LibAsset.sol:102-108
Timestamp: 2025-04-28T07:46:24.084Z
Learning: In the LiFi contracts, the `LibAsset.transferFromERC20` function is intentionally designed to accept arbitrary `from` parameters, with security ensured through a whitelist mechanism that controls which contracts can call this function.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-08-27T23:36:40.773Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: test/solidity/Periphery/GasZipPeriphery.t.sol:223-233
Timestamp: 2025-08-27T23:36:40.773Z
Learning: In bridge facet swap flows with requiresDeposit=false, tokens are already held by the diamond contract from previous swaps. When subsequent contracts like GasZipPeriphery call LibAsset.depositAsset, they pull tokens from msg.sender (the diamond) to themselves via transferFrom, requiring no additional approvals since the diamond already owns the tokens.

Applied to files:

src/Periphery/TokenWrapper.sol

🪛 GitHub Check: Olympix Integrated Security

src/Periphery/TokenWrapper.sol

[notice] 36-36:
Parameters passed to a constructor that are not validated for correct values may lead to contract creation in an undesired state. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/no-parameter-validation-in-constructor

🔇 Additional comments (1)

test/solidity/Periphery/TokenWrapper.t.sol (1)

8-27: New converter and decimal‑converter tests align well with the updated TokenWrapper

The additional setup and tests exercise the key new paths:

tokenWrapperWithConverter + TestWrappedConverter verify that ETH→ERC20 and ERC20→ETH flows work via the converter and that converter allowances are granted only when needed (testConverterReceivesApproval).

tokenWrapperWithDecimalConverter + TestConverterWithDecimals correctly model 18↔6 decimal conversions, including a full round-trip (testDecimalConverterRoundTrip), and assert balances in the right units (1e6 vs 1e18).

This gives solid coverage for both the WETH9-compatible and converter-based behaviors introduced in TokenWrapper.

Also applies to: 39-66, 100-215

src/Periphery/TokenWrapper.sol

coderabbitai

Actionable comments posted: 2

♻️ Duplicate comments (1)

src/Periphery/TokenWrapper.sol (1)

34-43: Constructor logic is correct for optional converter support.

The handling of _converter being address(0) is intentional and correctly sets USE_CONVERTER = false for the non-converter path. The static analysis warning is a false positive - based on learnings, the deployment flow validates constructor arguments through deployRequirements.json.

🧹 Nitpick comments (1)

src/Periphery/TokenWrapper.sol (1)
1-1: License identifier should be LGPL-3.0-only for LI.FI code.

As per coding guidelines, all LI.FI Solidity files should use LGPL-3.0-only license identifier.
-// SPDX-License-Identifier: UNLICENSED
+// SPDX-License-Identifier: LGPL-3.0-only

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 633c97f and 0379dff.

📒 Files selected for processing (1)

src/Periphery/TokenWrapper.sol (3 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

src/**/*.sol

📄 CodeRabbit inference engine (conventions.md)

src/**/*.sol: All Solidity files must start with SPDX-License-Identifier comment (LGPL-3.0-only for LI.FI code), followed by a blank line, then pragma statement
Use LGPL-3.0-only license identifier for all LI.FI Solidity files, with exceptions for external dependencies and copied contracts which must retain original license and note modifications
Error names must be descriptive, follow PascalCase, and not include error messages (for gas optimization). Use custom error types rather than generic revert() statements
Use camelCase for state variables (e.g., userBalance, tokenAddress), camelCase with underscore prefix for function parameters (e.g., _amount, _recipient), CONSTANT_CASE for constants and immutable variables (e.g., MAX_FEE, RELAY_DEPOSITORY), and camelCase for function names (e.g., startBridge, validateInput)
Every contract must have NatSpec documentation with @title, @author (always 'LI.FI (https://li.fi)'), @notice, and @Custom:version (semantic versioning X.Y.Z) tags in that specific order
Every public/external function must have NatSpec documentation including description, @param for each parameter, and @return for return values. Note if function is restricted to admin or specific addresses
Include single blank lines between logical sections (state variables, events, constructor, functions), between function declarations, before new logical blocks inside functions, and before return statements. No blank lines between function signature and body or between if statements and their revert() calls. No blank line between related consecutive mappings

Files:

src/Periphery/TokenWrapper.sol

🧠 Learnings (20)

📓 Common learnings

Learnt from: ezynda3
Repo: lifinance/contracts PR: 1124
File: script/demoScripts/utils/lib/cowShedSdk.ts:0-0
Timestamp: 2025-06-05T14:50:17.275Z
Learning: For demo scripts and example code, ezynda3 prefers to keep the code simple and focused on demonstrating the core functionality rather than adding extensive input validation or defensive programming measures.

📚 Learning: 2024-09-23T01:48:39.325Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 807
File: src/Periphery/GasZipPeriphery.sol:29-32
Timestamp: 2024-09-23T01:48:39.325Z
Learning: In our codebase, input validation for zero addresses in constructor parameters is not required because our deployment flow ensures that contracts cannot be deployed with zero address parameters unless we are okay with it.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-06-16T09:28:18.027Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1207
File: script/deploy/zksync/DeployPioneerFacet.zksync.s.sol:29-37
Timestamp: 2025-06-16T09:28:18.027Z
Learning: In the LIFI contracts codebase, constructor arguments in Solidity deployment scripts do not need validation because they are already validated by bash scripts in combination with the deployRequirements.json file. The deployRequirements.json file contains validation rules like "allowToDeployWithZeroAddress": false to prevent invalid constructor arguments during deployment.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-06-24T08:13:01.516Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1234
File: script/deploy/facets/DeployRelayFacet.s.sol:31-34
Timestamp: 2025-06-24T08:13:01.516Z
Learning: In the lifinance/contracts repository, parameter validation (like checking for address(0) or missing configuration values) should not be suggested in Foundry deploy scripts (files in script/deploy/) because the deployment process includes pre-validation through deployRequirements.json that checks these parameters before the Foundry scripts are executed. The deploy scripts can safely assume parameters are valid.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-02-11T10:33:52.791Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 988
File: script/deploy/facets/DeployPermit2Proxy.s.sol:33-37
Timestamp: 2025-02-11T10:33:52.791Z
Learning: The Permit2Proxy contract must not be deployed with zero addresses for its critical dependencies (LiFiDiamond, permit2Address, safeAddress). This is enforced by passing `false` to `_getConfigContractAddress` function.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-07-17T04:21:50.790Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1266
File: script/deploy/safe/execute-pending-timelock-tx.ts:129-129
Timestamp: 2025-07-17T04:21:50.790Z
Learning: In the lifinance/contracts repository, 0xDEnYO prefers not to add private key format validation in deployment scripts like execute-pending-timelock-tx.ts, prioritizing code simplicity over strict validation in their controlled environment.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-08-29T11:12:02.184Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: src/Periphery/LDA/Facets/SyncSwapV2Facet.sol:33-38
Timestamp: 2025-08-29T11:12:02.184Z
Learning: In LDA facets (src/Periphery/Lda/Facets/**/*.sol), tokenIn parameter validation (checking for address(0)) is intentionally not performed as part of the design architecture.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-07-16T01:03:08.106Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1275
File: src/Facets/AllBridgeFacet.sol:164-164
Timestamp: 2025-07-16T01:03:08.106Z
Learning: In src/Facets/AllBridgeFacet.sol, the team has decided that explicit validation for address downcasting from `_bridgeData.sendingAssetId` to `bytes32(uint256(uint160(_bridgeData.sendingAssetId)))` is not required, accepting the potential risk of silent overflow from unsafe downcasting.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-06-05T14:28:34.813Z

Learnt from: ezynda3
Repo: lifinance/contracts PR: 1124
File: script/demoScripts/demoPatcher.ts:0-0
Timestamp: 2025-06-05T14:28:34.813Z
Learning: For demo scripts in the lifinance/contracts repository, simpler validation (like checking for zero address) is acceptable and additional contract code validation is not necessary.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-07-17T04:21:55.549Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1266
File: script/deploy/safe/execute-pending-timelock-tx.ts:334-334
Timestamp: 2025-07-17T04:21:55.549Z
Learning: In the lifinance/contracts repository, 0xDEnYO prefers to keep private key processing simple in scripts like execute-pending-timelock-tx.ts without adding format validation, prioritizing code simplicity over strict validation in controlled environments.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-07-17T04:21:26.825Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1266
File: script/deploy/safe/execute-pending-timelock-tx.ts:627-628
Timestamp: 2025-07-17T04:21:26.825Z
Learning: In the lifinance/contracts repository, 0xDEnYO prefers to keep '0x0' as a fallback address in gas estimation calls rather than throwing errors when the wallet account address is not available, prioritizing code simplicity over strict validation.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2024-10-14T08:23:38.076Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 831
File: src/Helpers/WithdrawablePeriphery.sol:26-30
Timestamp: 2024-10-14T08:23:38.076Z
Learning: In the `WithdrawablePeriphery.sol` contract, when transferring Ether in the `withdrawToken` function, prefer using `SafeTransferLib.safeTransferETH` instead of low-level `call` for safer Ether transfers.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-01-23T02:26:29.900Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 946
File: config/networks.json:260-260
Timestamp: 2025-01-23T02:26:29.900Z
Learning: Each network in the configuration has its own unique wrapped native token address specified in the `wrappedNativeAddress` field. These addresses are network-specific and should not be expected to be the same or available across different networks.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-01-28T14:26:29.054Z

Learnt from: ezynda3
Repo: lifinance/contracts PR: 924
File: script/deploy/zksync/DeployExecutor.s.sol:48-48
Timestamp: 2025-01-28T14:26:29.054Z
Learning: The Executor contract constructor accepts two parameters:
1. _erc20Proxy: The address of the ERC20Proxy contract
2. _owner: The owner address (passed to WithdrawablePeriphery)

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-09-25T00:12:58.536Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1395
File: src/Periphery/FeeForwarder.sol:78-120
Timestamp: 2025-09-25T00:12:58.536Z
Learning: In src/Periphery/FeeForwarder.sol, the team intentionally uses address(this).balance for refunding remaining native tokens because the contract is designed to never hold funds and not collect dust, making it safe to return the entire balance to the caller.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-08-20T09:56:24.259Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1327
File: docs/OutputValidator.md:23-29
Timestamp: 2025-08-20T09:56:24.259Z
Learning: In src/Periphery/OutputValidator.sol, the ERC20 token flow that uses balanceOf(msg.sender) to compute actualAmount and transfers the excess via safeTransferFrom is intentional behavior, as confirmed by 0xDEnYO. The design choice to potentially include pre-existing ERC20 balances in the excess calculation is by design and should not be flagged as a security concern in future reviews.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-04-28T07:46:24.084Z

Learnt from: ezynda3
Repo: lifinance/contracts PR: 1116
File: src/Libraries/LibAsset.sol:102-108
Timestamp: 2025-04-28T07:46:24.084Z
Learning: In the LiFi contracts, the `LibAsset.transferFromERC20` function is intentionally designed to accept arbitrary `from` parameters, with security ensured through a whitelist mechanism that controls which contracts can call this function.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-08-27T23:36:40.773Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: test/solidity/Periphery/GasZipPeriphery.t.sol:223-233
Timestamp: 2025-08-27T23:36:40.773Z
Learning: In bridge facet swap flows with requiresDeposit=false, tokens are already held by the diamond contract from previous swaps. When subsequent contracts like GasZipPeriphery call LibAsset.depositAsset, they pull tokens from msg.sender (the diamond) to themselves via transferFrom, requiring no additional approvals since the diamond already owns the tokens.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-08-28T22:41:58.814Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: test/solidity/Periphery/Lda/BaseCoreRouteTest.t.sol:387-395
Timestamp: 2025-08-28T22:41:58.814Z
Learning: In the lifinance/contracts repository, SafeERC20's safeApprove() method handles the two-step approval pattern internally when needed, so manual reset-to-zero followed by approval is unnecessary. Simply changing from IERC20.approve() to IERC20.safeApprove() provides the safety benefits.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2024-10-10T03:18:20.721Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 782
File: test/solidity/Helpers/WithdrawablePeriphery.t.sol:22-35
Timestamp: 2024-10-10T03:18:20.721Z
Learning: In Solidity tests for withdrawal functions in `test/solidity/Helpers/WithdrawablePeriphery.t.sol`, do not suggest adding tests where the withdrawal amount exceeds the contract's balance, as such tests are unnecessary because any contract will fail in that case.

Applied to files:

src/Periphery/TokenWrapper.sol

🪛 GitHub Check: Olympix Integrated Security

src/Periphery/TokenWrapper.sol

[notice] 36-36:
Parameters passed to a constructor that are not validated for correct values may lead to contract creation in an undesired state. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/no-parameter-validation-in-constructor

🔇 Additional comments (3)

src/Periphery/TokenWrapper.sol (3)

18-27: Good use of immutable variables for gas optimization and security.

The state variable design is clean - using immutable for all configuration values ensures they cannot be modified after deployment and saves gas on reads.

74-86: Withdrawal logic correctly handles both converter and non-converter paths.

The balance-before/after pattern properly accounts for any differences in the converted amount, and the approval is correctly scoped to the converter. Good use of SafeTransferLib.safeTransferETH for native token transfers.

89-91: LGTM!

The receive() function is necessary to accept ETH from the wrapped token's withdraw() call.

src/Periphery/TokenWrapper.sol

lifi-action-bot · 2025-12-12T10:36:03Z

🤖 GitHub Action: Security Alerts Review 🔍

🟢 Dismissed Security Alerts with Comments
The following alerts were dismissed with proper comments:

🟢 View Alert - File: src/Periphery/TokenWrapper.sol
🔹 Parameters passed to a constructor that are not validated for correct values may lead to contract creation in an undesired state. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/no-parameter-validation-in-constructor
🔹 Dismiss Reason: Won't fix
🔹 Dismiss Comment: Not needed

🟢 View Alert - File: src/Periphery/TokenWrapper.sol
🔹 Using a payable fallback (including receive) with no explicit functionality may indicate incomplete contract logic. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/empty-payable-fallback
🔹 Dismiss Reason: Won't fix
🔹 Dismiss Comment: Needs to be payable to receive unwrapped native

🟢 View Alert - File: src/Helpers/TransferrableOwnership.sol
🔹 Parameters passed to a constructor that are not validated for correct values may lead to contract creation in an undesired state. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/no-parameter-validation-in-constructor
🔹 Dismiss Reason: Won't fix
🔹 Dismiss Comment: Since it’s a minor issue, we don’t need to validate the owner. Moreover, the contract can be marked as abstract. Newly added facets will generate similar alerts, and we will have the opportunity to validate the value before it reaches the parent contract.

🟢 View Alert - File: src/Helpers/WithdrawablePeriphery.sol
🔹 Parameters passed to a constructor that are not validated for correct values may lead to contract creation in an undesired state. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/no-parameter-validation-in-constructor
🔹 Dismiss Reason: Won't fix
🔹 Dismiss Comment: Since it’s a minor issue, we don’t need to validate the owner. Moreover, the contract can be marked as abstract. Newly added facets will generate similar alerts, and we will have the opportunity to validate the value before it reaches the parent contract.

✅ No unresolved security alerts! 🎉

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

coderabbitai

Actionable comments posted: 0

♻️ Duplicate comments (1)

src/Periphery/TokenWrapper.sol (1)

18-41: Immutable configuration and wrapper/converter routing are well-structured

Using immutables for WRAPPED_TOKEN, CONVERTER, WRAPPER_ADDRESS, and USE_CONVERTER, with WRAPPER_ADDRESS set to either the converter or the wrapped token based on _converter != address(0), gives a clear and gas-efficient split between the direct-wrapper and converter-backed modes. This matches how the updated tests and deploy scripts intend to use TokenWrapper, and is consistent with the repository’s pattern of relying on external deployment validation instead of in-contract constructor parameter checks.

🧹 Nitpick comments (3)

test/solidity/Periphery/TokenWrapper.t.sol (1)

163-216: Decimal-converter tests thoroughly validate 18↔6 conversion; consider a minor extra assertion

The decimal tests validate:

1 ETH → 1e6 units of the 6-dec token on deposit.

1e6 units → 1 ETH on withdraw.

Round-trip behavior for 5 ETH using both deposit and withdraw.

To tighten the round-trip test slightly, you could also assert that token6Decimals.balanceOf(address(this)) == 0 after the final withdraw to mirror the non-decimal converter test pattern, but this is purely optional.

src/Periphery/TokenWrapper.sol (2)

12-16: Align contract-level NatSpec order with guidelines and reconcile version bump with audit check

Per the repo’s Solidity guidelines, the contract header should order tags as @title, @notice, @author, @custom:version. Here @author precedes @notice; consider swapping those two lines to be consistent with the standard format.

Also, the custom version was bumped to 1.2.0 and the pipeline reports no logged audit for this version. Please ensure the audit records/config are updated accordingly (or adjust the version) so the VersionControlAndAuditVerification check passes.

45-99: Deposit/withdraw logic correctly handles both direct-wrapper and converter paths; consider a tiny withdraw(0) optimization

The updated deposit and withdraw implementations:

Correctly use balance-before/after deltas in the converter path so that decimal conversions or fees don’t assume 1:1 behavior.

Keep the non-converter path equivalent to the original WETH9-style 1:1 wrap/unwrap semantics.

Use SafeTransferLib consistently for ERC20 and ETH transfers, which addresses prior concerns about raw transfer/transferFrom.

As a minor optional optimization, you could early-return in withdraw() when wad == 0 to avoid a no-op approval and external withdraw(0) call in edge cases, but functionally the current code is fine.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 2d5148b and 3ffe919.

📒 Files selected for processing (8)

script/deploy/facets/DeployTokenWrapper.s.sol (3 hunks)
script/deploy/zksync/DeployTokenWrapper.zksync.s.sol (3 hunks)
src/Interfaces/IWrapper.sol (1 hunks)
src/Periphery/TokenWrapper.sol (1 hunks)
test/solidity/Periphery/TokenWrapper.t.sol (2 hunks)
test/solidity/utils/TestBasicToken.sol (1 hunks)
test/solidity/utils/TestConverterWithDecimals.sol (1 hunks)
test/solidity/utils/TestWrappedConverter.sol (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (3)

test/solidity/utils/TestBasicToken.sol
script/deploy/facets/DeployTokenWrapper.s.sol
script/deploy/zksync/DeployTokenWrapper.zksync.s.sol

🧰 Additional context used

📓 Path-based instructions (4)

test/**/*.sol

📄 CodeRabbit inference engine (conventions.md)

All Solidity test files must start with SPDX-License-Identifier comment (LGPL-3.0-only), followed by a blank line, then pragma statement

Files:

test/solidity/utils/TestWrappedConverter.sol
test/solidity/Periphery/TokenWrapper.t.sol
test/solidity/utils/TestConverterWithDecimals.sol

src/**/*.sol

📄 CodeRabbit inference engine (conventions.md)

src/**/*.sol: All Solidity files must start with SPDX-License-Identifier comment (LGPL-3.0-only for LI.FI code), followed by a blank line, then pragma statement
Use LGPL-3.0-only license identifier for all LI.FI Solidity files, with exceptions for external dependencies and copied contracts which must retain original license and note modifications
Error names must be descriptive, follow PascalCase, and not include error messages (for gas optimization). Use custom error types rather than generic revert() statements
Use camelCase for state variables (e.g., userBalance, tokenAddress), camelCase with underscore prefix for function parameters (e.g., _amount, _recipient), CONSTANT_CASE for constants and immutable variables (e.g., MAX_FEE, RELAY_DEPOSITORY), and camelCase for function names (e.g., startBridge, validateInput)
Every contract must have NatSpec documentation with @title, @author (always 'LI.FI (https://li.fi)'), @notice, and @Custom:version (semantic versioning X.Y.Z) tags in that specific order
Every public/external function must have NatSpec documentation including description, @param for each parameter, and @return for return values. Note if function is restricted to admin or specific addresses
Include single blank lines between logical sections (state variables, events, constructor, functions), between function declarations, before new logical blocks inside functions, and before return statements. No blank lines between function signature and body or between if statements and their revert() calls. No blank line between related consecutive mappings

Files:

src/Interfaces/IWrapper.sol
src/Periphery/TokenWrapper.sol

src/Interfaces/*.sol

📄 CodeRabbit inference engine (conventions.md)

src/Interfaces/*.sol: All interfaces must start with I prefix (e.g., ILiFi, IStargate) and be placed in separate files in src/Interfaces directory, not in the same file as their implementation
Every interface must have NatSpec documentation with @title, @notice, @author (always 'LI.FI (https://li.fi)'), and @Custom:version (semantic versioning X.Y.Z) tags in that specific order

Files:

src/Interfaces/IWrapper.sol

test/**/*.t.sol

📄 CodeRabbit inference engine (conventions.md)

test/**/*.t.sol: Test files must have .t.sol extension. All successful tests must be prefixed with test_, failure/revert tests prefixed with testRevert_, and inherited tests prefixed with testBase_. Every test contract must have setUp() function that calls initTestBase() if inheriting from TestBase
In test files, use blank lines between vm.expectRevert() and function call, before vm.stopPrank() if a separate logical block, and before assertions. Single blank line between test cases, after vm.startPrank(address), and before/after vm.expectEmit blocks (no blank line between vm.expectEmit and its event definition)
Use vm.expectRevert() to verify specific revert reasons in failure test cases (simply checking success/failure status of call() is not sufficient). Use vm.expectEmit(true, true, true, true, ) for event testing
For facets requiring whitelist functionality, test contracts should inherit from TestWhitelistManagerBase and use its functions (addToWhitelist, removeFromWhitelist, setFunctionWhitelistBySelector, removeFunctionApprovalBySelector)

Files:

test/solidity/Periphery/TokenWrapper.t.sol

🧠 Learnings (40)

📓 Common learnings

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 831
File: src/Helpers/WithdrawablePeriphery.sol:26-30
Timestamp: 2024-10-14T08:23:38.076Z
Learning: In the `WithdrawablePeriphery.sol` contract, when transferring Ether in the `withdrawToken` function, prefer using `SafeTransferLib.safeTransferETH` instead of low-level `call` for safer Ether transfers.

Learnt from: ezynda3
Repo: lifinance/contracts PR: 1124
File: script/demoScripts/utils/lib/cowShedSdk.ts:0-0
Timestamp: 2025-06-05T14:50:17.275Z
Learning: For demo scripts and example code, ezynda3 prefers to keep the code simple and focused on demonstrating the core functionality rather than adding extensive input validation or defensive programming measures.

📚 Learning: 2024-10-10T03:18:20.721Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 782
File: test/solidity/Helpers/WithdrawablePeriphery.t.sol:22-35
Timestamp: 2024-10-10T03:18:20.721Z
Learning: In Solidity tests for withdrawal functions in `test/solidity/Helpers/WithdrawablePeriphery.t.sol`, do not suggest adding tests where the withdrawal amount exceeds the contract's balance, as such tests are unnecessary because any contract will fail in that case.

Applied to files:

test/solidity/utils/TestWrappedConverter.sol
test/solidity/Periphery/TokenWrapper.t.sol
src/Periphery/TokenWrapper.sol
test/solidity/utils/TestConverterWithDecimals.sol

📚 Learning: 2024-10-14T08:23:38.076Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 831
File: src/Helpers/WithdrawablePeriphery.sol:26-30
Timestamp: 2024-10-14T08:23:38.076Z
Learning: In the `WithdrawablePeriphery.sol` contract, when transferring Ether in the `withdrawToken` function, prefer using `SafeTransferLib.safeTransferETH` instead of low-level `call` for safer Ether transfers.

Applied to files:

test/solidity/utils/TestWrappedConverter.sol
src/Interfaces/IWrapper.sol
test/solidity/Periphery/TokenWrapper.t.sol
src/Periphery/TokenWrapper.sol
test/solidity/utils/TestConverterWithDecimals.sol

📚 Learning: 2025-12-02T10:08:06.478Z

Learnt from: CR
Repo: lifinance/contracts PR: 0
File: conventions.md:0-0
Timestamp: 2025-12-02T10:08:06.478Z
Learning: Applies to test/**/*.t.sol : For facets requiring whitelist functionality, test contracts should inherit from TestWhitelistManagerBase and use its functions (addToWhitelist, removeFromWhitelist, setFunctionWhitelistBySelector, removeFunctionApprovalBySelector)

Applied to files:

test/solidity/utils/TestWrappedConverter.sol
test/solidity/Periphery/TokenWrapper.t.sol

📚 Learning: 2025-06-07T04:11:20.100Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1141
File: test/solidity/Periphery/LidoWrapper.t.sol:55-70
Timestamp: 2025-06-07T04:11:20.100Z
Learning: On Layer 2 networks, Lido's stETH contract has inverted function naming compared to expected behavior: `unwrap()` converts stETH → wstETH, and `wrap()` converts wstETH → stETH. This is the opposite of what the function names suggest but is how Lido implemented their L2 contracts.

Applied to files:

test/solidity/utils/TestWrappedConverter.sol
src/Periphery/TokenWrapper.sol

📚 Learning: 2025-08-29T11:53:38.549Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: test/solidity/Periphery/LDA/BaseCoreRouteTest.t.sol:379-388
Timestamp: 2025-08-29T11:53:38.549Z
Learning: In test/solidity/Periphery/LDA/BaseCoreRouteTest.t.sol, for the revert-testing helper function _executeAndVerifySwap, only the aggregator branch (CommandType.DistributeSelfERC20) should use amountIn-1 to underfund and trigger insufficient balance errors, while user-funded branches should use the full amountIn to test other error conditions.

Applied to files:

test/solidity/utils/TestWrappedConverter.sol
test/solidity/Periphery/TokenWrapper.t.sol
src/Periphery/TokenWrapper.sol
test/solidity/utils/TestConverterWithDecimals.sol

📚 Learning: 2025-12-02T10:08:06.478Z

Learnt from: CR
Repo: lifinance/contracts PR: 0
File: conventions.md:0-0
Timestamp: 2025-12-02T10:08:06.478Z
Learning: Applies to src/Interfaces/*.sol : All interfaces must start with I prefix (e.g., ILiFi, IStargate) and be placed in separate files in src/Interfaces directory, not in the same file as their implementation

Applied to files:

src/Interfaces/IWrapper.sol

📚 Learning: 2025-12-12T11:16:09.236Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 1530
File: src/Interfaces/ICircleBridgeProxy.sol:20-29
Timestamp: 2025-12-12T11:16:09.236Z
Learning: In src/Interfaces/ICircleBridgeProxy.sol, ensure the ICircleBridgeProxy interface corresponds to Celer's CircleBridgeProxy contract (not Circle's native TokenMessenger). The depositForBurn signature must be: depositForBurn(uint256 _amount, uint64 _dstChid, bytes32 _mintRecipient, address _burnToken, uint256 _maxFee, uint32 _minFinalityThreshold) external. This change should be scoped specifically to this file; pattern is not needed since it targets a single file. This improves correctness by aligning the interface with the correct contract and parameter types (dst chain id as uint64, no bytes32 destinationCaller parameter).

Applied to files:

src/Interfaces/IWrapper.sol
src/Periphery/TokenWrapper.sol

📚 Learning: 2025-08-27T22:23:51.257Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: src/Periphery/Lda/Facets/NativeWrapperFacet.sol:0-0
Timestamp: 2025-08-27T22:23:51.257Z
Learning: In src/Periphery/Lda/Facets/NativeWrapperFacet.sol, the wrapNative function should not validate msg.value == amountIn because CoreRouteFacet's DistributeNative command (command type 3) overrides the amountIn parameter with address(this).balance, making such validation incorrect and causing false failures.

Applied to files:

test/solidity/Periphery/TokenWrapper.t.sol
src/Periphery/TokenWrapper.sol

📚 Learning: 2025-08-29T11:33:37.368Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: test/solidity/Periphery/LDA/BaseCoreRouteTest.t.sol:1-3
Timestamp: 2025-08-29T11:33:37.368Z
Learning: For test files in test/solidity/Periphery/LDA/**, the SPDX license identifier should be immediately followed by the pragma statement without a blank line in between, as confirmed by mirooon for test/solidity/Periphery/LDA/BaseCoreRouteTest.t.sol.

Applied to files:

test/solidity/Periphery/TokenWrapper.t.sol

📚 Learning: 2024-10-09T03:47:21.269Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 807
File: src/Periphery/GasZipPeriphery.sol:4-14
Timestamp: 2024-10-09T03:47:21.269Z
Learning: In `GasZipPeriphery.sol`, `LibUtil` and `Validatable` are used, so ensure not to suggest their removal in future reviews.

Applied to files:

test/solidity/Periphery/TokenWrapper.t.sol

📚 Learning: 2025-08-27T22:28:47.277Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: test/solidity/Periphery/Lda/Facets/SyncSwapV2Facet.t.sol:278-304
Timestamp: 2025-08-27T22:28:47.277Z
Learning: The _buildRouteAndExecuteSwap helper function in LDA tests (like in test/solidity/Periphery/Lda/BaseCoreRouteTest.t.sol and similar) handles token approvals internally, so manual tokenIn.approve() calls are not needed in individual test functions when using this helper.

Applied to files:

test/solidity/Periphery/TokenWrapper.t.sol

📚 Learning: 2025-01-28T11:29:09.566Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 945
File: test/solidity/Facets/GlacisFacet.t.sol:0-0
Timestamp: 2025-01-28T11:29:09.566Z
Learning: Empty test methods with explanatory comments are acceptable in test classes when the feature being tested (e.g., native token bridging) is not supported by the implementation.

Applied to files:

test/solidity/Periphery/TokenWrapper.t.sol

📚 Learning: 2025-01-22T12:38:37.557Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 945
File: test/solidity/utils/TestBase.sol:446-470
Timestamp: 2025-01-22T12:38:37.557Z
Learning: Test utility functions in TestBase.sol don't require production-level validations like slippage checks and input validation since they operate in a controlled test environment.

Applied to files:

test/solidity/Periphery/TokenWrapper.t.sol

📚 Learning: 2025-08-28T02:41:07.505Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1321
File: docs/RelayDepositoryFacet.md:9-10
Timestamp: 2025-08-28T02:41:07.505Z
Learning: Unit tests for RelayDepositoryFacet cannot verify fund forwarding behavior after deposits because the facet delegates to external IRelayDepository contracts. The forwarding logic is implemented in the Relay Protocol V2 Depository contracts, not in the facet itself.

Applied to files:

test/solidity/Periphery/TokenWrapper.t.sol

📚 Learning: 2025-08-29T11:07:57.743Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: src/Periphery/LDA/Facets/CurveFacet.sol:87-91
Timestamp: 2025-08-29T11:07:57.743Z
Learning: In src/Periphery/LDA/Facets/CurveFacet.sol, modern Curve pools (isV2=true, representing V2/NG pools) should reject native tokenIn by adding an early revert check when LibAsset.isNativeAsset(tokenIn) is true, since ICurveV2 exchange functions are non-payable and cannot accept native ETH.

Applied to files:

test/solidity/Periphery/TokenWrapper.t.sol
src/Periphery/TokenWrapper.sol

📚 Learning: 2025-08-27T22:29:00.940Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: test/solidity/Periphery/Lda/Facets/SyncSwapV2Facet.t.sol:312-323
Timestamp: 2025-08-27T22:29:00.940Z
Learning: In the LDA test suite, the `_buildRouteAndExecuteSwap` helper function already handles token approvals internally, so manual approval calls are not needed in individual test functions.

Applied to files:

test/solidity/Periphery/TokenWrapper.t.sol

📚 Learning: 2025-01-22T12:36:12.699Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 945
File: test/solidity/Facets/GlacisFacet.t.sol:214-262
Timestamp: 2025-01-22T12:36:12.699Z
Learning: The GlacisFacet test suite inherits from TestBaseFacet which already covers various failure scenarios including invalid receiver address, invalid amounts, same chain bridging, and insufficient funds, making additional failure scenario tests redundant.

Applied to files:

test/solidity/Periphery/TokenWrapper.t.sol

📚 Learning: 2025-11-04T16:27:25.773Z

Learnt from: RnkSngh
Repo: lifinance/contracts PR: 1422
File: src/Interfaces/ITokenMessenger.sol:1-3
Timestamp: 2025-11-04T16:27:25.773Z
Learning: For external/third-party interfaces in src/Interfaces/ (e.g., ITokenMessenger.sol for Polymer/CCTP), the team does not require following the project's NatSpec, licensing (LGPL-3.0-only), or pragma conventions, as these interfaces represent contracts the team doesn't control.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-08-29T11:18:56.656Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: src/Periphery/LDA/Facets/NativeWrapperFacet.sol:1-2
Timestamp: 2025-08-29T11:18:56.656Z
Learning: For src/Periphery/LDA/Facets/NativeWrapperFacet.sol, the SPDX license identifier should be immediately followed by the pragma statement without a blank line in between, as confirmed by mirooon.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2024-09-23T01:48:39.325Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 807
File: src/Periphery/GasZipPeriphery.sol:29-32
Timestamp: 2024-09-23T01:48:39.325Z
Learning: In our codebase, input validation for zero addresses in constructor parameters is not required because our deployment flow ensures that contracts cannot be deployed with zero address parameters unless we are okay with it.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-06-16T09:28:18.027Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1207
File: script/deploy/zksync/DeployPioneerFacet.zksync.s.sol:29-37
Timestamp: 2025-06-16T09:28:18.027Z
Learning: In the LIFI contracts codebase, constructor arguments in Solidity deployment scripts do not need validation because they are already validated by bash scripts in combination with the deployRequirements.json file. The deployRequirements.json file contains validation rules like "allowToDeployWithZeroAddress": false to prevent invalid constructor arguments during deployment.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-06-24T08:13:01.516Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1234
File: script/deploy/facets/DeployRelayFacet.s.sol:31-34
Timestamp: 2025-06-24T08:13:01.516Z
Learning: In the lifinance/contracts repository, parameter validation (like checking for address(0) or missing configuration values) should not be suggested in Foundry deploy scripts (files in script/deploy/) because the deployment process includes pre-validation through deployRequirements.json that checks these parameters before the Foundry scripts are executed. The deploy scripts can safely assume parameters are valid.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-02-11T10:33:52.791Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 988
File: script/deploy/facets/DeployPermit2Proxy.s.sol:33-37
Timestamp: 2025-02-11T10:33:52.791Z
Learning: The Permit2Proxy contract must not be deployed with zero addresses for its critical dependencies (LiFiDiamond, permit2Address, safeAddress). This is enforced by passing `false` to `_getConfigContractAddress` function.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-07-17T04:21:50.790Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1266
File: script/deploy/safe/execute-pending-timelock-tx.ts:129-129
Timestamp: 2025-07-17T04:21:50.790Z
Learning: In the lifinance/contracts repository, 0xDEnYO prefers not to add private key format validation in deployment scripts like execute-pending-timelock-tx.ts, prioritizing code simplicity over strict validation in their controlled environment.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-08-29T11:12:02.184Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: src/Periphery/LDA/Facets/SyncSwapV2Facet.sol:33-38
Timestamp: 2025-08-29T11:12:02.184Z
Learning: In LDA facets (src/Periphery/Lda/Facets/**/*.sol), tokenIn parameter validation (checking for address(0)) is intentionally not performed as part of the design architecture.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-07-16T01:03:08.106Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1275
File: src/Facets/AllBridgeFacet.sol:164-164
Timestamp: 2025-07-16T01:03:08.106Z
Learning: In src/Facets/AllBridgeFacet.sol, the team has decided that explicit validation for address downcasting from `_bridgeData.sendingAssetId` to `bytes32(uint256(uint160(_bridgeData.sendingAssetId)))` is not required, accepting the potential risk of silent overflow from unsafe downcasting.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-06-05T14:28:34.813Z

Learnt from: ezynda3
Repo: lifinance/contracts PR: 1124
File: script/demoScripts/demoPatcher.ts:0-0
Timestamp: 2025-06-05T14:28:34.813Z
Learning: For demo scripts in the lifinance/contracts repository, simpler validation (like checking for zero address) is acceptable and additional contract code validation is not necessary.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-07-17T04:21:55.549Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1266
File: script/deploy/safe/execute-pending-timelock-tx.ts:334-334
Timestamp: 2025-07-17T04:21:55.549Z
Learning: In the lifinance/contracts repository, 0xDEnYO prefers to keep private key processing simple in scripts like execute-pending-timelock-tx.ts without adding format validation, prioritizing code simplicity over strict validation in controlled environments.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-06-05T15:04:58.212Z

Learnt from: ezynda3
Repo: lifinance/contracts PR: 1124
File: script/demoScripts/utils/cowSwapHelpers.ts:112-114
Timestamp: 2025-06-05T15:04:58.212Z
Learning: For demo scripts in the lifinance/contracts repository, security concerns like using Math.random() for nonce generation are acceptable since the focus is on demonstrating functionality rather than production-ready security implementation.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-08-20T09:56:24.259Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1327
File: docs/OutputValidator.md:23-29
Timestamp: 2025-08-20T09:56:24.259Z
Learning: In src/Periphery/OutputValidator.sol, the ERC20 token flow that uses balanceOf(msg.sender) to compute actualAmount and transfers the excess via safeTransferFrom is intentional behavior, as confirmed by 0xDEnYO. The design choice to potentially include pre-existing ERC20 balances in the excess calculation is by design and should not be flagged as a security concern in future reviews.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2024-09-23T02:04:16.323Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 807
File: src/Periphery/GasZipPeriphery.sol:49-53
Timestamp: 2024-09-23T02:04:16.323Z
Learning: When `LibAsset.maxApproveERC20` is used with `type(uint256).max`, the team acknowledges and accepts the associated security risks. In future reviews, avoid flagging this as a concern.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-08-26T12:11:21.073Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: src/Periphery/Lda/Facets/CoreRouteFacet.sol:4-6
Timestamp: 2025-08-26T12:11:21.073Z
Learning: In the lifinance/contracts repository, OpenZeppelin's SafeERC20 library provides safePermit functionality for IERC20Permit tokens. This is confirmed by working implementations in both CoreRouteFacet.sol and LiFiDEXAggregator.sol that use the pattern "using SafeERC20 for IERC20Permit;" and successfully call safePermit with permit parameters.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-08-26T12:11:21.073Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: src/Periphery/Lda/Facets/CoreRouteFacet.sol:4-6
Timestamp: 2025-08-26T12:11:21.073Z
Learning: OpenZeppelin's SafeERC20 library provides safePermit functionality for IERC20Permit tokens in modern versions. The usage pattern "using SafeERC20 for IERC20Permit;" is valid and enables safe permit operations. This is confirmed to be working in the lifinance/contracts codebase.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-08-26T12:11:21.073Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: src/Periphery/Lda/Facets/CoreRouteFacet.sol:4-6
Timestamp: 2025-08-26T12:11:21.073Z
Learning: OpenZeppelin's SafeERC20 library provides safePermit functionality for IERC20Permit tokens. The using SafeERC20 for IERC20Permit; directive enables calling safePermit on IERC20Permit tokens safely.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-08-26T12:11:21.073Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: src/Periphery/Lda/Facets/CoreRouteFacet.sol:4-6
Timestamp: 2025-08-26T12:11:21.073Z
Learning: OpenZeppelin's SafeERC20 library provides safePermit functionality for IERC20Permit tokens. The pattern "using SafeERC20 for IERC20Permit;" enables calling safePermit on IERC20Permit tokens safely. This functionality is available in recent versions of OpenZeppelin contracts.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-08-27T23:01:41.042Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: src/Libraries/LibUniV3Logic.sol:4-5
Timestamp: 2025-08-27T23:01:41.042Z
Learning: In the lifinance/contracts repository, the pattern `import { IERC20, SafeERC20 } from "openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";` is correctly used throughout the codebase and works as expected. SafeERC20.sol re-exports IERC20 in modern OpenZeppelin versions, making this combined import valid. Do not suggest splitting these imports.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-08-26T12:11:21.073Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: src/Periphery/Lda/Facets/CoreRouteFacet.sol:4-6
Timestamp: 2025-08-26T12:11:21.073Z
Learning: OpenZeppelin's SafeERC20 library provides safePermit functionality for IERC20Permit tokens. This is confirmed by actual usage in the lifinance/contracts codebase where safePermit is used in multiple files including CoreRouteFacet.sol and LiFiDEXAggregator.sol. The pattern "using SafeERC20 for IERC20Permit;" is valid and working.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-09-25T00:12:58.536Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1395
File: src/Periphery/FeeForwarder.sol:78-120
Timestamp: 2025-09-25T00:12:58.536Z
Learning: In src/Periphery/FeeForwarder.sol, the team intentionally uses address(this).balance for refunding remaining native tokens because the contract is designed to never hold funds and not collect dust, making it safe to return the entire balance to the caller.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-01-23T02:26:29.900Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 946
File: config/networks.json:260-260
Timestamp: 2025-01-23T02:26:29.900Z
Learning: Each network in the configuration has its own unique wrapped native token address specified in the `wrappedNativeAddress` field. These addresses are network-specific and should not be expected to be the same or available across different networks.

Applied to files:

src/Periphery/TokenWrapper.sol

🪛 GitHub Actions: VersionControlAndAuditVerification

src/Periphery/TokenWrapper.sol

[error] 1-1: Could not find any logged audit for contract TokenWrapper in version 1.2.0.

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)

GitHub Check: run-unit-tests
GitHub Check: enforce-min-test-coverage

🔇 Additional comments (5)

src/Interfaces/IWrapper.sol (1)

5-13: IWrapper definition matches WETH9-style interface and project conventions

License, pragma, interface-level NatSpec, and function signatures (including uint wad with the solhint override) are all consistent with the intended WETH9-compatible usage in TokenWrapper. No changes needed.

test/solidity/utils/TestWrappedConverter.sol (1)

7-42: TestWrappedConverter correctly models a WETH9-compatible converter for TokenWrapper tests

Deposit/withdraw flows, error handling, and the receive hook line up with how TokenWrapper interacts with a converter (ETH in, wrapped tokens out; wrapped tokens in, ETH out). This is appropriate as a focused test helper.

test/solidity/utils/TestConverterWithDecimals.sol (1)

7-54: Decimal conversion helper behaves consistently with GasUSDT0-style semantics

The 1e12 scaling, divisibility check in deposit, and mirrored conversion in withdraw give predictable 18↔6 decimal behavior that matches the new TokenWrapper decimal-converter tests. This is suitable as a dedicated test utility.

test/solidity/Periphery/TokenWrapper.t.sol (2)

31-67: Constructor wiring for all three TokenWrapper instances looks correct

tokenWrapper, tokenWrapperWithConverter, and tokenWrapperWithDecimalConverter all pass (wrappedToken, converterOrZero, owner) in line with the updated 3-arg constructor. Dealing and minting to the converter contracts ensures they have sufficient ERC20 and ETH balances for the deposit/withdraw test flows.

101-161: Converter-path tests exercise both deposit and withdraw, including allowance behavior

The new tests for the converter case (test_CanDepositWithConverter, test_CanWithdrawWithConverter, test_ConverterReceivesApproval) correctly:

Verify 1:1 token credit on deposit via the converter-backed wrapper.

Cover the full withdraw path (tokens pulled from the user, ETH forwarded back).

Assert that LibAsset.maxApproveERC20 results in type(uint256).max allowance from the wrapper to the converter.

This gives good coverage of the new USE_CONVERTER branch and approval logic.

archive/config/tokenwrapper.json

script/deploy/facets/DeployTokenWrapper.s.sol

script/deploy/resources/deployRequirements.json

test/solidity/Periphery/TokenWrapper.t.sol

test/solidity/utils/TestBasicToken.sol

archive/config/tokenwrapper.json

ezynda3

Fixed dcb8a6d: File deleted, uses networks.json

src/Periphery/TokenWrapper.sol

0xDEnYO · 2025-12-17T00:01:52Z

config/networks.json

+    "nativeAddress": "0x0000000000000000000000000000000000000000",
+    "nativeCurrency": "gUSDT",
+    "wrappedNativeAddress": "0x779ded0c9e1022225f8e0630b35a9b54be713736",
+    "converterAddress": "0xded1660192d4d82e7c0b628ba556861edbb5cada",


if you add this here, we also need to adjust the type in script/common/types.ts

However, I am not sure if this is the right place. I feel a config file for tokenWrapper.json (only containing the converter entry for stable) would be the cleaner approach. But that's just an opinion

So we actually archived the tokenwrapper.json file in favor of networks.json (that's why it appeared in the archive folder). Not sure we should bring it back if we don't use like 90% of it anyway...
@mirooon what do you think?

@ezynda3 I think it's more tokenWrapper-specific, I would keep it in tokenWrapper.json

@ezynda3 to clarify on this:
I was not suggesting to unarchive the file as-is.
It is absolutely correct to pull the wrappedNative address from networks.json to avoid duplicate content.
But the tokenWrapper.json file can just contain the converter address (and only for networks for which one exists), so it would currently be only one entry: stable

created a separate config for converter addresses tokenconverters.json

src/Periphery/TokenWrapper.sol

0xDEnYO · 2025-12-17T00:05:15Z

src/Periphery/TokenWrapper.sol

-        IWrapper(wrappedToken).deposit{ value: msg.value }();
-        IERC20(wrappedToken).transfer(msg.sender, msg.value);
+        if (USE_CONVERTER) {
+            uint256 balanceBefore = IERC20(WRAPPED_TOKEN).balanceOf(


yes I like it

- Create new config/tokenconverters.json for token converter addresses - Update DeployTokenWrapper.s.sol to read from tokenconverters.json - Remove converterAddress from networks.json stable chain entry - Defaults to address(0) for chains without a converter configured

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (1)

script/deploy/facets/DeployTokenWrapper.s.sol (1)

35-50: Silent fallback for missing converter address aligns with sparse configuration design, but differs from other config patterns in the script.

The try/catch at lines 35-50 is intentional: tokenconverters.json currently contains only the "stable" network, so the try/catch gracefully defaults to address(0) for the 66 other networks without explicit converter configuration.

However:

Lines 52-56 only validate non-zero addresses; address(0) passes through without validation against the tokenconverters.json key presence.

Other config reads in the same script (e.g., wrappedNativeAddress at line 30) use _getConfigContractAddress() which fails explicitly on missing values, not silently falling back.

This inconsistency could mask real JSON structure issues (malformed syntax, typos in "converterAddress" key) if the file is accidentally corrupted.

Consider whether all config reads should follow the same strict pattern (explicit failure) or if sparse config support with fallback is intentional. If intentional, add validation to ensure tokenconverters.json is valid JSON even if the network key is missing.

📜 Review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 59c8de4 and 8cf10c2.

📒 Files selected for processing (3)

config/networks.json
config/tokenconverters.json
script/deploy/facets/DeployTokenWrapper.s.sol

✅ Files skipped from review due to trivial changes (1)

config/tokenconverters.json

🧰 Additional context used

📓 Path-based instructions (7)

script/deploy/**/*.s.sol

📄 CodeRabbit inference engine (.cursor/rules/001-project-structure.mdc)

Place deployment scripts in script/deploy/facets/ for facet deployments or script/deploy/ for other contracts. Use Foundry's Script contract pattern.

Files:

script/deploy/facets/DeployTokenWrapper.s.sol

**/*.sol

📄 CodeRabbit inference engine (.cursor/rules/001-project-structure.mdc)

Use remappings defined in remappings.txt for imports: lifi/ → src/, test/ → test/, and external libs like @openzeppelin/, solmate/, solady/, permit2/

**/*.sol: Single Diamond (EIP-2535) as main entrypoint for all protocol interactions
Delegate complex logic to libraries (LibAsset, LibSwap, LibAllowList, SwapperV2, Validatable) and helper contracts

After Solidity changes, run forge test (or note suites remaining)

**/*.sol: Own files must use // SPDX-License-Identifier: LGPL-3.0-only immediately followed by the pragma statement with no blank line in between
All contracts must use pragma solidity ^0.8.17;
Functions and variables use camelCase; constants and immutables are CONSTANT_CASE
Function parameters use leading underscore (e.g., _amount)
Contracts and interfaces must include NatSpec: @title (matching contract/interface name), @author LI.FI (https://li.fi), @notice describing purpose, and @custom:version X.Y.Z
Public and external functions require NatSpec including params and returns documentation
For pure test/script scaffolding keep NatSpec headers minimal but retain SPDX and pragma
Use single blank lines between logical sections and between function declarations
Follow in-function blank-line rules: blank lines before emits/returns; no stray gaps
Use custom errors instead of revert strings; prefer existing errors/helpers before adding new ones
Use generic errors from src/Errors/GenericErrors.sol; bump @custom:version when adding; facet-specific errors stay local
Adhere to rules in .solhint.json
Avoid assembly unless necessary and heavily commented with justification (why assembly is needed); prefer existing helpers over new implementations

Do not mix interface and implementation in the same file

Files:

script/deploy/facets/DeployTokenWrapper.s.sol

{src,script,test}/**/*.{sol,ts}

📄 CodeRabbit inference engine (.cursor/rules/105-security.mdc)

Validate all external inputs and configuration (including script/env values) explicitly; prefer existing validation helpers (e.g., Validatable, config readers) over ad-hoc checks

Files:

script/deploy/facets/DeployTokenWrapper.s.sol

{src,script}/**/*.sol

📄 CodeRabbit inference engine (.cursor/rules/105-security.mdc)

{src,script}/**/*.sol: Do not weaken existing access controls, timelock flows, or Safe multisig protections; any change that touches admin-only functionality must call out its governance impact
Avoid introducing new external call patterns in facets or scripts without checking existing libraries (LibAsset, LibSwap, LibAllowList) for prior art

Files:

script/deploy/facets/DeployTokenWrapper.s.sol

script/**/{Deploy,Update}*.s.sol

📄 CodeRabbit inference engine (.cursor/rules/107-solidity-scripts.mdc)

Use Foundry deploy/update script patterns with Deploy*.s.sol or Update*.s.sol naming and JSON config via stdJson

Files:

script/deploy/facets/DeployTokenWrapper.s.sol

script/**/Deploy*.s.sol

📄 CodeRabbit inference engine (.cursor/rules/107-solidity-scripts.mdc)

Deploy scripts should use DeployScriptBase pattern with deploy(type(Contract).creationCode) and only add getConstructorArgs() when needed, reading from JSON via stdJson

Files:

script/deploy/facets/DeployTokenWrapper.s.sol

script/deploy/**/*.sol

📄 CodeRabbit inference engine (.cursor/rules/107-solidity-scripts.mdc)

Keep deployment scripts under script/deploy/ directory structure (and script/deploy/zksync/ when relevant) and reuse helper/config files like deployRequirements.json and targetState.json instead of creating new patterns

Files:

script/deploy/facets/DeployTokenWrapper.s.sol

🧠 Learnings (45)

📓 Common learnings

Learnt from: ezynda3
Repo: lifinance/contracts PR: 1124
File: script/demoScripts/utils/lib/cowShedSdk.ts:0-0
Timestamp: 2025-06-05T14:50:17.275Z
Learning: For demo scripts and example code, ezynda3 prefers to keep the code simple and focused on demonstrating the core functionality rather than adding extensive input validation or defensive programming measures.

📚 Learning: 2025-12-10T17:36:54.412Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 1524
File: config/whitelist.json:4358-4366
Timestamp: 2025-12-10T17:36:54.412Z
Learning: Repo lifinance/contracts: In config/whitelist.json, temporary network keys (e.g., "stable") may appear under DEX entries (such as Fly) while deployments are in progress and the corresponding chain configuration PR hasn’t merged yet. This is expected, does not break the workflow, and should not be flagged as an unsupported chain key.

Applied to files:

config/networks.json

📚 Learning: 2024-11-21T08:34:30.300Z

Learnt from: ezynda3
Repo: lifinance/contracts PR: 861
File: config/global.json:146-146
Timestamp: 2024-11-21T08:34:30.300Z
Learning: The project is deprecating `safeAddresses` and `safeApiUrls` in `global.json` and moving these configurations to `config/networks.json` for network configurations.

Applied to files:

config/networks.json

📚 Learning: 2024-10-08T07:14:52.296Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 825
File: config/networks.json:462-462
Timestamp: 2024-10-08T07:14:52.296Z
Learning: In the `networks.json` file and related configuration files, when updating the `name` property of a network (e.g., from `Scroll` to `zkScroll`), the key of the network object should remain unchanged (e.g., `'scroll'`) to ensure it matches references elsewhere in the codebase.

Applied to files:

config/networks.json

📚 Learning: 2024-11-21T08:24:05.881Z

Learnt from: ezynda3
Repo: lifinance/contracts PR: 861
File: script/deploy/safe/add-owners-to-safe.ts:8-13
Timestamp: 2024-11-21T08:24:05.881Z
Learning: In `script/deploy/safe/add-owners-to-safe.ts`, validation for network configuration when casting imported JSON data to `NetworksObject` is not required as per the user's preference.

Applied to files:

config/networks.json

📚 Learning: 2025-07-17T11:31:50.058Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 1283
File: config/networks.json:837-839
Timestamp: 2025-07-17T11:31:50.058Z
Learning: In config/networks.json for the Ronin network (chainId 2020), there is a problematic mismatch between verificationType ("blockscout") and explorerApiUrl ("https://sourcify.roninchain.com/server"). This inconsistency can lead to verification issues and should be flagged when networks.json is modified in future PRs.

Applied to files:

config/networks.json

📚 Learning: 2024-11-18T07:19:28.651Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 858
File: config/networks.json:17-17
Timestamp: 2024-11-18T07:19:28.651Z
Learning: In the `config/networks.json` file, it's expected that the `safeWebUrl` field may have different formats and URLs across networks; standardizing them is not desired.

Applied to files:

config/networks.json

📚 Learning: 2024-11-22T08:50:28.878Z

Learnt from: ezynda3
Repo: lifinance/contracts PR: 827
File: config/networks.json:448-448
Timestamp: 2024-11-22T08:50:28.878Z
Learning: In `config/networks.json`, Safe addresses (`safeAddress` fields) do not need to be verified, as they are handled separately.

Applied to files:

config/networks.json

📚 Learning: 2025-08-28T02:02:56.965Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1136
File: config/dexs.json:0-0
Timestamp: 2025-08-28T02:02:56.965Z
Learning: Different networks in config/dexs.json naturally and appropriately have different DEX addresses. Do not flag different addresses across different networks as issues - only flag if the same network's addresses appear to have changed unintentionally between branches.

Applied to files:

config/networks.json

📚 Learning: 2025-05-27T12:36:26.987Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1168
File: script/deploy/_targetState.json:1564-1589
Timestamp: 2025-05-27T12:36:26.987Z
Learning: When reviewing deployment PRs in the lifinance/contracts repository, target state configuration files (like script/deploy/_targetState.json) may be updated for multiple networks even when the PR is focused on deploying to a specific network. The scope should be determined by the PR title and description, not just by all configuration changes present in the files.

Applied to files:

config/networks.json

📚 Learning: 2025-12-17T10:29:06.122Z

Learnt from: CR
Repo: lifinance/contracts PR: 0
File: .cursor/rules/200-typescript.mdc:0-0
Timestamp: 2025-12-17T10:29:06.122Z
Learning: Applies to script/common/types.ts : Type placement for shared/cross-cutting types: Place in `script/common/types.ts` (e.g., `SupportedChain`, `INetwork`, `EnvironmentEnum`).

Applied to files:

config/networks.json

📚 Learning: 2025-12-17T10:29:06.121Z

Learnt from: CR
Repo: lifinance/contracts PR: 0
File: .cursor/rules/200-typescript.mdc:0-0
Timestamp: 2025-12-17T10:29:06.121Z
Learning: Type placement for domain-specific types: Colocate in domain-specific files (e.g., Tron types in `script/deploy/tron/types.ts`, Safe types in `script/deploy/safe/safe-utils.ts`).

Applied to files:

config/networks.json

📚 Learning: 2025-12-17T10:29:06.121Z

Learnt from: CR
Repo: lifinance/contracts PR: 0
File: .cursor/rules/200-typescript.mdc:0-0
Timestamp: 2025-12-17T10:29:06.121Z
Learning: Always check for existing types before defining new ones: Search in order: TypeChain types (`typechain/`), viem types, `script/common/types.ts`, domain-specific type files (e.g., `script/troncast/types.ts`, `script/deploy/tron/types.ts`).

Applied to files:

config/networks.json

📚 Learning: 2025-01-23T02:26:29.900Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 946
File: config/networks.json:260-260
Timestamp: 2025-01-23T02:26:29.900Z
Learning: Each network in the configuration has its own unique wrapped native token address specified in the `wrappedNativeAddress` field. These addresses are network-specific and should not be expected to be the same or available across different networks.

Applied to files:

config/networks.json
script/deploy/facets/DeployTokenWrapper.s.sol

📚 Learning: 2024-12-04T01:59:34.045Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 832
File: deployments/_deployments_log_file.json:23712-23720
Timestamp: 2024-12-04T01:59:34.045Z
Learning: In `deployments/_deployments_log_file.json`, duplicate deployment entries for the same version and address may occur because they correspond to deployments on different networks. These entries are acceptable and should not be flagged as duplicates in future reviews.

Applied to files:

config/networks.json

📚 Learning: 2024-10-21T01:27:47.808Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 836
File: config/dexs.json:396-397
Timestamp: 2024-10-21T01:27:47.808Z
Learning: In `config/dexs.json`, it's acceptable for a whitelisted DEX address to be the same across multiple networks.

Applied to files:

config/networks.json

📚 Learning: 2024-10-09T03:47:21.269Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 819
File: deployments/scroll.diamond.json:82-82
Timestamp: 2024-10-09T03:47:21.269Z
Learning: In the `deployments/*.diamond.json` and `deployments/*.json` files, the `LiFiDEXAggregator` contract may intentionally have the same contract address across multiple networks. This is acceptable and should not be flagged as an issue in future code reviews.

Applied to files:

config/networks.json
script/deploy/facets/DeployTokenWrapper.s.sol

📚 Learning: 2025-02-13T03:04:19.306Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 992
File: script/deploy/_targetState.json:25-32
Timestamp: 2025-02-13T03:04:19.306Z
Learning: Duplicate contract names (e.g., ERC20Proxy, Executor, etc.) in _targetState.json are expected when they appear across different network configurations, as they represent the same contract type deployed on multiple networks.

Applied to files:

config/networks.json

📚 Learning: 2025-12-17T10:28:51.722Z

Learnt from: CR
Repo: lifinance/contracts PR: 0
File: .cursor/rules/107-solidity-scripts.mdc:0-0
Timestamp: 2025-12-17T10:28:51.722Z
Learning: Applies to script/**/Deploy*.s.sol : Deploy scripts should use `DeployScriptBase` pattern with `deploy(type(Contract).creationCode)` and only add `getConstructorArgs()` when needed, reading from JSON via `stdJson`

Applied to files:

script/deploy/facets/DeployTokenWrapper.s.sol

📚 Learning: 2025-08-26T15:19:07.800Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: script/deploy/facets/LDA/DeployAlgebraFacet.s.sol:4-4
Timestamp: 2025-08-26T15:19:07.800Z
Learning: DeployScriptBase.sol is located at script/deploy/facets/utils/DeployScriptBase.sol, not script/deploy/utils/DeployScriptBase.sol. Import paths from script/deploy/facets/LDA/ should use "../utils/DeployScriptBase.sol" to reference it correctly.

Applied to files:

script/deploy/facets/DeployTokenWrapper.s.sol

📚 Learning: 2025-12-17T10:28:03.630Z

Learnt from: CR
Repo: lifinance/contracts PR: 0
File: .cursor/rules/101-solidity-contracts.mdc:0-0
Timestamp: 2025-12-17T10:28:03.630Z
Learning: Applies to script/deploy/resources/deployRequirements.json : When adding a new deployable contract that has constructor arguments, add/update its entry in `script/deploy/resources/deployRequirements.json` so deploy scripts validate the presence of required constructor args.

Applied to files:

script/deploy/facets/DeployTokenWrapper.s.sol

📚 Learning: 2025-04-21T03:17:53.443Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1109
File: deployments/worldchain.json:28-28
Timestamp: 2025-04-21T03:17:53.443Z
Learning: For deployment PRs updating contract addresses (like RelayFacet on Worldchain), verify the presence of entries in all relevant files (worldchain.json, worldchain.diamond.json, _deployments_log_file.json) before reporting inconsistencies. The RelayFacet entry exists in all required deployment files with the correct address.

Applied to files:

script/deploy/facets/DeployTokenWrapper.s.sol

📚 Learning: 2025-07-04T08:59:08.108Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1256
File: deployments/zksync.diamond.json:81-87
Timestamp: 2025-07-04T08:59:08.108Z
Learning: When analyzing deployment PRs in the lifinance/contracts repository, carefully verify that target state configuration files (like script/deploy/_targetState.json) have been updated before flagging missing entries. The AI summary section should be consulted to understand all file changes, as manual searches might miss entries due to formatting differences or search limitations.

Applied to files:

script/deploy/facets/DeployTokenWrapper.s.sol

📚 Learning: 2025-12-17T10:27:21.083Z

Learnt from: CR
Repo: lifinance/contracts PR: 0
File: .cursor/rules/002-architecture.mdc:0-0
Timestamp: 2025-12-17T10:27:21.083Z
Learning: Applies to **/*.sol : Delegate complex logic to libraries (LibAsset, LibSwap, LibAllowList, SwapperV2, Validatable) and helper contracts

Applied to files:

script/deploy/facets/DeployTokenWrapper.s.sol

📚 Learning: 2024-11-25T13:49:40.464Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 782
File: script/deploy/zksync/DeployPermit2Proxy.s.sol:22-61
Timestamp: 2024-11-25T13:49:40.464Z
Learning: In the deploy scripts (e.g., `script/deploy/zksync/DeployPermit2Proxy.s.sol`), complex error handling and address validation are not necessary. The team prefers to keep deploy scripts simple without extensive error handling.

Applied to files:

script/deploy/facets/DeployTokenWrapper.s.sol

📚 Learning: 2025-12-17T10:28:13.904Z

Learnt from: CR
Repo: lifinance/contracts PR: 0
File: .cursor/rules/102-facets.mdc:0-0
Timestamp: 2025-12-17T10:28:13.904Z
Learning: Applies to src/Facets/**/*Facet.sol : Facets must use LibAsset, LibSwap, and LibAllowList utilities along with Validatable and SwapperV2 interfaces; reserve native fees via `_depositAndSwap` variants when needed

Applied to files:

script/deploy/facets/DeployTokenWrapper.s.sol

📚 Learning: 2025-10-02T18:14:45.047Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 1406
File: script/deploy/facets/UpdateUnitFacet.s.sol:1-3
Timestamp: 2025-10-02T18:14:45.047Z
Learning: For update scripts in script/deploy/facets/, the SPDX license identifier should be immediately followed by the pragma statement without a blank line in between, as confirmed by mirooon for script/deploy/facets/UpdateUnitFacet.s.sol.

Applied to files:

script/deploy/facets/DeployTokenWrapper.s.sol

📚 Learning: 2025-08-07T10:20:01.383Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 1283
File: deployments/ronin.diamond.json:65-68
Timestamp: 2025-08-07T10:20:01.383Z
Learning: When analyzing deployment PRs in the lifinance/contracts repository, carefully verify that target state configuration files (like script/deploy/_targetState.json) and deployment log files have been updated before flagging missing entries. The AI summary section should be consulted to understand all file changes, as manual searches might miss entries due to formatting differences or search limitations.

Applied to files:

script/deploy/facets/DeployTokenWrapper.s.sol

📚 Learning: 2024-09-23T01:48:39.325Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 807
File: src/Periphery/GasZipPeriphery.sol:29-32
Timestamp: 2024-09-23T01:48:39.325Z
Learning: In our codebase, input validation for zero addresses in constructor parameters is not required because our deployment flow ensures that contracts cannot be deployed with zero address parameters unless we are okay with it.

Applied to files:

script/deploy/facets/DeployTokenWrapper.s.sol

📚 Learning: 2025-02-12T09:44:10.963Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 985
File: deployments/bsca.json:0-0
Timestamp: 2025-02-12T09:44:10.963Z
Learning: When setting up new network deployments, it's expected that contract addresses in the deployment JSON files (e.g., bsca.json, bscb.json) may not have deployed code initially, as these files serve as target configurations for future deployments.

Applied to files:

script/deploy/facets/DeployTokenWrapper.s.sol

📚 Learning: 2025-07-17T04:21:26.825Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1266
File: script/deploy/safe/execute-pending-timelock-tx.ts:627-628
Timestamp: 2025-07-17T04:21:26.825Z
Learning: In the lifinance/contracts repository, 0xDEnYO prefers to keep '0x0' as a fallback address in gas estimation calls rather than throwing errors when the wallet account address is not available, prioritizing code simplicity over strict validation.

Applied to files:

script/deploy/facets/DeployTokenWrapper.s.sol

📚 Learning: 2024-11-26T01:03:43.597Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 782
File: deployments/scroll.diamond.json:111-111
Timestamp: 2024-11-26T01:03:43.597Z
Learning: When reviewing pull requests, only point out missing contract addresses in `deployments/<network>.diamond.json` if the contract's address is present in `deployments/<network>.json` but missing in `deployments/<network>.diamond.json`. If the contract is not deployed on a network (i.e., not present in `deployments/<network>.json`), then no action is needed.

Applied to files:

script/deploy/facets/DeployTokenWrapper.s.sol

📚 Learning: 2025-08-07T06:34:07.709Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1316
File: script/deploy/deployAllContracts.sh:346-350
Timestamp: 2025-08-07T06:34:07.709Z
Learning: In script/deploy/deployAllContracts.sh, 0xDEnYO prefers not to add automated retry mechanisms for cast send commands in deployment scripts, preferring manual re-execution if network issues occur. This prioritizes simplicity over automated error handling.

Applied to files:

script/deploy/facets/DeployTokenWrapper.s.sol

📚 Learning: 2024-10-09T03:47:21.269Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 819
File: deployments/moonriver.diamond.json:68-68
Timestamp: 2024-10-09T03:47:21.269Z
Learning: In the lifinance/contracts repository, it's acceptable that not every contract is deployed on all networks, so empty addresses for contracts not deployed on a network are acceptable in deployment files.

Applied to files:

script/deploy/facets/DeployTokenWrapper.s.sol

📚 Learning: 2025-04-21T03:15:12.236Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1109
File: deployments/worldchain.diamond.json:84-85
Timestamp: 2025-04-21T03:15:12.236Z
Learning: In deployment JSON files that contain "diamond" in their filename (located in the deployments folder), periphery contracts may have empty string values for their addresses. This indicates that the contract is not deployed on that particular chain and should not be flagged as an issue during code reviews.

Applied to files:

script/deploy/facets/DeployTokenWrapper.s.sol

📚 Learning: 2025-02-24T09:35:34.908Z

Learnt from: ezynda3
Repo: lifinance/contracts PR: 984
File: src/Periphery/ReceiverChainflip.sol:48-62
Timestamp: 2025-02-24T09:35:34.908Z
Learning: Contract address validations for periphery contracts like ReceiverChainflip are handled in deployment scripts rather than in the constructor.

Applied to files:

script/deploy/facets/DeployTokenWrapper.s.sol

📚 Learning: 2025-01-28T14:27:50.689Z

Learnt from: ezynda3
Repo: lifinance/contracts PR: 924
File: script/deploy/zksync/DeployReceiverStargateV2.s.sol:19-21
Timestamp: 2025-01-28T14:27:50.689Z
Learning: In LiFi's deployment scripts, the `deploy` method in `DeployScriptBase` handles the concatenation of constructor arguments with the contract's creation code, so child contracts don't need to concatenate the arguments explicitly.

Applied to files:

script/deploy/facets/DeployTokenWrapper.s.sol

📚 Learning: 2025-09-25T00:04:16.437Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1395
File: script/deploy/zksync/DeployFeeForwarder.zksync.s.sol:4-4
Timestamp: 2025-09-25T00:04:16.437Z
Learning: ZkSync deployment scripts have their own DeployScriptBase.sol located at script/deploy/zksync/utils/DeployScriptBase.sol, and all ZkSync deployment scripts use the relative import path "./utils/DeployScriptBase.sol" to reference it, not "../facets/utils/DeployScriptBase.sol".

Applied to files:

script/deploy/facets/DeployTokenWrapper.s.sol

📚 Learning: 2025-01-30T02:51:43.006Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 963
File: script/deploy/zksync/DeploySymbiosisFacet.zksync.s.sol:21-34
Timestamp: 2025-01-30T02:51:43.006Z
Learning: Parameter validation for deployment scripts in the LiFi contracts is handled by pre-deployment scripts, making additional validation within individual deployment scripts unnecessary.

Applied to files:

script/deploy/facets/DeployTokenWrapper.s.sol

📚 Learning: 2025-03-05T14:41:05.791Z

Learnt from: ezynda3
Repo: lifinance/contracts PR: 953
File: script/deploy/zksync/utils/UpdateScriptBase.sol:32-50
Timestamp: 2025-03-05T14:41:05.791Z
Learning: In the LiFi contracts repository, explicit error handling for file operations and address validation in deployment scripts like UpdateScriptBase.sol is not considered necessary by the maintainer.

Applied to files:

script/deploy/facets/DeployTokenWrapper.s.sol

📚 Learning: 2025-12-17T10:28:51.722Z

Learnt from: CR
Repo: lifinance/contracts PR: 0
File: .cursor/rules/107-solidity-scripts.mdc:0-0
Timestamp: 2025-12-17T10:28:51.722Z
Learning: Applies to script/**/{Deploy,Update}*.s.sol : Use Foundry deploy/update script patterns with `Deploy*.s.sol` or `Update*.s.sol` naming and JSON config via `stdJson`

Applied to files:

script/deploy/facets/DeployTokenWrapper.s.sol

📚 Learning: 2025-06-16T09:28:18.027Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1207
File: script/deploy/zksync/DeployPioneerFacet.zksync.s.sol:29-37
Timestamp: 2025-06-16T09:28:18.027Z
Learning: In the LIFI contracts codebase, constructor arguments in Solidity deployment scripts do not need validation because they are already validated by bash scripts in combination with the deployRequirements.json file. The deployRequirements.json file contains validation rules like "allowToDeployWithZeroAddress": false to prevent invalid constructor arguments during deployment.

Applied to files:

script/deploy/facets/DeployTokenWrapper.s.sol

📚 Learning: 2024-10-09T03:47:21.269Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 819
File: deployments/avalanche.diamond.json:105-105
Timestamp: 2024-10-09T03:47:21.269Z
Learning: In the `lifinance/contracts` repository, contracts may have different addresses across networks. Do not check if contracts have the same addresses across all networks in future reviews.

Applied to files:

script/deploy/facets/DeployTokenWrapper.s.sol

📚 Learning: 2025-09-25T00:12:58.536Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1395
File: src/Periphery/FeeForwarder.sol:78-120
Timestamp: 2025-09-25T00:12:58.536Z
Learning: In src/Periphery/FeeForwarder.sol, the team intentionally uses address(this).balance for refunding remaining native tokens because the contract is designed to never hold funds and not collect dust, making it safe to return the entire balance to the caller.

Applied to files:

script/deploy/facets/DeployTokenWrapper.s.sol

📚 Learning: 2025-10-13T11:13:48.847Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 1413
File: src/Facets/EverclearV2Facet.sol:75-96
Timestamp: 2025-10-13T11:13:48.847Z
Learning: In EverclearV2Facet (and all LiFi facets), the team standard is to use msg.sender as the refund address with the refundExcessNative modifier, not requiring an explicit refund address parameter.

Applied to files:

script/deploy/facets/DeployTokenWrapper.s.sol

🔇 Additional comments (4)

config/networks.json (1)

1072-1090: No action needed. The "stable" network entry contains expected placeholder values for a newly configured network. All active mainnet networks have zero addresses for multicall and Safe until deployment occurs and contracts are initialized. The missing create3Factory field is also normal—other networks like "abstract", "lens", and "sophon" similarly lack this field. The gasZipChainId of 0 indicates the network is not yet participating in the GasZip protocol, which is appropriate for a new network. These configurations will be updated as deployment progresses.

script/deploy/facets/DeployTokenWrapper.s.sol (3)

8-9: LGTM!

Imports are appropriate for the contract validation logic. Using InvalidContract from GenericErrors and LibAsset for contract checks aligns with project patterns.

52-56: LGTM!

Contract validation is well-implemented. Using LibAsset.isContract() to verify the converter address and reverting with the generic InvalidContract error follows project patterns and prevents deployment with invalid converter addresses.

72-77: LGTM!

Constructor args correctly updated to include the new converterAddress parameter. The three-argument encoding aligns with the TokenWrapper constructor changes and matches the pattern used in other deployment scripts (zkSync variant).

script/deploy/facets/DeployTokenWrapper.s.sol

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

…ontracts into feat/token-wrapper-converter

Replace balance-before/after pattern with immutable SWAP_RATIO_MULTIPLIER that is calculated once in constructor based on wrapped token decimals. This provides gas savings by eliminating balance checks and calculations, ensures deterministic conversion amounts, and simplifies the code by removing conditional branches.

coderabbitai

Actionable comments posted: 2

📜 Review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between b3b2dba and bc81b9c.

📒 Files selected for processing (1)

src/Periphery/TokenWrapper.sol

🧰 Additional context used

📓 Path-based instructions (5)

src/Periphery/**/*.sol

📄 CodeRabbit inference engine (.cursor/rules/001-project-structure.mdc)

Place new periphery contracts in the src/Periphery/ directory. These are standalone contracts called by the Diamond but are not facets and can be upgraded independently.

Periphery contracts handle specialized flows that don't belong directly in the Diamond

Files:

src/Periphery/TokenWrapper.sol

**/*.sol

📄 CodeRabbit inference engine (.cursor/rules/001-project-structure.mdc)

Use remappings defined in remappings.txt for imports: lifi/ → src/, test/ → test/, and external libs like @openzeppelin/, solmate/, solady/, permit2/

**/*.sol: Single Diamond (EIP-2535) as main entrypoint for all protocol interactions
Delegate complex logic to libraries (LibAsset, LibSwap, LibAllowList, SwapperV2, Validatable) and helper contracts

After Solidity changes, run forge test (or note suites remaining)

**/*.sol: Own files must use // SPDX-License-Identifier: LGPL-3.0-only immediately followed by the pragma statement with no blank line in between
All contracts must use pragma solidity ^0.8.17;
Functions and variables use camelCase; constants and immutables are CONSTANT_CASE
Function parameters use leading underscore (e.g., _amount)
Contracts and interfaces must include NatSpec: @title (matching contract/interface name), @author LI.FI (https://li.fi), @notice describing purpose, and @custom:version X.Y.Z
Public and external functions require NatSpec including params and returns documentation
For pure test/script scaffolding keep NatSpec headers minimal but retain SPDX and pragma
Use single blank lines between logical sections and between function declarations
Follow in-function blank-line rules: blank lines before emits/returns; no stray gaps
Use custom errors instead of revert strings; prefer existing errors/helpers before adding new ones
Use generic errors from src/Errors/GenericErrors.sol; bump @custom:version when adding; facet-specific errors stay local
Adhere to rules in .solhint.json
Avoid assembly unless necessary and heavily commented with justification (why assembly is needed); prefer existing helpers over new implementations

Do not mix interface and implementation in the same file

Files:

src/Periphery/TokenWrapper.sol

src/**/*.sol

📄 CodeRabbit inference engine (.cursor/rules/101-solidity-contracts.mdc)

src/**/*.sol: NatSpec required on contracts/interfaces in src/: @title, @author LI.FI (https://li.fi), @notice, @custom:version X.Y.Z; document all public/external functions with params/returns.
Bug bounty clarity: In contract NatSpec, explicitly document whether the contract is intended to hold/custody funds. If it is not designed to hold funds, state that clearly (e.g., "This contract is not intended to custody user funds / hold balances; any funds held are incidental (e.g., transient during execution) and should not persist."). If it is designed to hold funds (e.g., FeeCollector), describe what funds it holds and under what conditions.
Apply Diamond patterns with existing libs (LibAsset/LibSwap/LibAllowList, Validatable/SwapperV2); prefer parameters over msg.sender for refund addresses.

src/**/*.sol: Follow gas-related best practices in existing facets before introducing new micro-optimizations in Solidity
Prefer using existing optimized libraries already in the repo (e.g., Solady/Solmate utilities) where they are in use, rather than hand-rolled low-level code
Do not sacrifice readability or safety for minor gas wins; explain any non-obvious optimization and justify it with expected impact
Avoid introducing inline assembly unless clearly needed; when used, document assumptions and invariants thoroughly

All contracts in src/ require audits except Interfaces in src/Interfaces/** (type definitions only) and external dependencies in lib/

Files:

src/Periphery/TokenWrapper.sol

{src,script,test}/**/*.{sol,ts}

📄 CodeRabbit inference engine (.cursor/rules/105-security.mdc)

Validate all external inputs and configuration (including script/env values) explicitly; prefer existing validation helpers (e.g., Validatable, config readers) over ad-hoc checks

Files:

src/Periphery/TokenWrapper.sol

{src,script}/**/*.sol

📄 CodeRabbit inference engine (.cursor/rules/105-security.mdc)

{src,script}/**/*.sol: Do not weaken existing access controls, timelock flows, or Safe multisig protections; any change that touches admin-only functionality must call out its governance impact
Avoid introducing new external call patterns in facets or scripts without checking existing libraries (LibAsset, LibSwap, LibAllowList) for prior art

Files:

src/Periphery/TokenWrapper.sol

🧠 Learnings (39)

📓 Common learnings

Learnt from: ezynda3
Repo: lifinance/contracts PR: 1124
File: script/demoScripts/utils/lib/cowShedSdk.ts:0-0
Timestamp: 2025-06-05T14:50:17.275Z
Learning: For demo scripts and example code, ezynda3 prefers to keep the code simple and focused on demonstrating the core functionality rather than adding extensive input validation or defensive programming measures.

📚 Learning: 2024-10-14T08:23:38.076Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 831
File: src/Helpers/WithdrawablePeriphery.sol:26-30
Timestamp: 2024-10-14T08:23:38.076Z
Learning: In the `WithdrawablePeriphery.sol` contract, when transferring Ether in the `withdrawToken` function, prefer using `SafeTransferLib.safeTransferETH` instead of low-level `call` for safer Ether transfers.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-12-17T10:28:34.379Z

Learnt from: CR
Repo: lifinance/contracts PR: 0
File: .cursor/rules/104-receiver-contracts.mdc:0-0
Timestamp: 2025-12-17T10:28:34.379Z
Learning: Applies to src/Periphery/Receiver*.sol : Receiver contracts must implement internal `_swapAndCompleteBridgeTokens()` function that handles native vs ERC20 tokens: use `safeApproveWithRetry()` or `safeIncreaseAllowance()` for ERC20 (reset to 0 after), pass `value: amount` for native tokens, call `executor.swapAndCompleteBridgeTokens()` in try-catch block, and on failure send raw tokens to receiver and emit `LiFiTransferRecovered`

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-08-29T11:18:56.656Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: src/Periphery/LDA/Facets/NativeWrapperFacet.sol:1-2
Timestamp: 2025-08-29T11:18:56.656Z
Learning: For src/Periphery/LDA/Facets/NativeWrapperFacet.sol, the SPDX license identifier should be immediately followed by the pragma statement without a blank line in between, as confirmed by mirooon.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-11-04T16:27:25.773Z

Learnt from: RnkSngh
Repo: lifinance/contracts PR: 1422
File: src/Interfaces/ITokenMessenger.sol:1-3
Timestamp: 2025-11-04T16:27:25.773Z
Learning: For external/third-party interfaces in src/Interfaces/ (e.g., ITokenMessenger.sol for Polymer/CCTP), the team does not require following the project's NatSpec, licensing (LGPL-3.0-only), or pragma conventions, as these interfaces represent contracts the team doesn't control.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2024-09-23T01:48:39.325Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 807
File: src/Periphery/GasZipPeriphery.sol:29-32
Timestamp: 2024-09-23T01:48:39.325Z
Learning: In our codebase, input validation for zero addresses in constructor parameters is not required because our deployment flow ensures that contracts cannot be deployed with zero address parameters unless we are okay with it.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-06-16T09:28:18.027Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1207
File: script/deploy/zksync/DeployPioneerFacet.zksync.s.sol:29-37
Timestamp: 2025-06-16T09:28:18.027Z
Learning: In the LIFI contracts codebase, constructor arguments in Solidity deployment scripts do not need validation because they are already validated by bash scripts in combination with the deployRequirements.json file. The deployRequirements.json file contains validation rules like "allowToDeployWithZeroAddress": false to prevent invalid constructor arguments during deployment.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-06-24T08:13:01.516Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1234
File: script/deploy/facets/DeployRelayFacet.s.sol:31-34
Timestamp: 2025-06-24T08:13:01.516Z
Learning: In the lifinance/contracts repository, parameter validation (like checking for address(0) or missing configuration values) should not be suggested in Foundry deploy scripts (files in script/deploy/) because the deployment process includes pre-validation through deployRequirements.json that checks these parameters before the Foundry scripts are executed. The deploy scripts can safely assume parameters are valid.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-02-11T10:33:52.791Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 988
File: script/deploy/facets/DeployPermit2Proxy.s.sol:33-37
Timestamp: 2025-02-11T10:33:52.791Z
Learning: The Permit2Proxy contract must not be deployed with zero addresses for its critical dependencies (LiFiDiamond, permit2Address, safeAddress). This is enforced by passing `false` to `_getConfigContractAddress` function.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-07-17T04:21:50.790Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1266
File: script/deploy/safe/execute-pending-timelock-tx.ts:129-129
Timestamp: 2025-07-17T04:21:50.790Z
Learning: In the lifinance/contracts repository, 0xDEnYO prefers not to add private key format validation in deployment scripts like execute-pending-timelock-tx.ts, prioritizing code simplicity over strict validation in their controlled environment.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-08-29T11:12:02.184Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: src/Periphery/LDA/Facets/SyncSwapV2Facet.sol:33-38
Timestamp: 2025-08-29T11:12:02.184Z
Learning: In LDA facets (src/Periphery/Lda/Facets/**/*.sol), tokenIn parameter validation (checking for address(0)) is intentionally not performed as part of the design architecture.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-07-16T01:03:08.106Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1275
File: src/Facets/AllBridgeFacet.sol:164-164
Timestamp: 2025-07-16T01:03:08.106Z
Learning: In src/Facets/AllBridgeFacet.sol, the team has decided that explicit validation for address downcasting from `_bridgeData.sendingAssetId` to `bytes32(uint256(uint160(_bridgeData.sendingAssetId)))` is not required, accepting the potential risk of silent overflow from unsafe downcasting.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-06-05T14:28:34.813Z

Learnt from: ezynda3
Repo: lifinance/contracts PR: 1124
File: script/demoScripts/demoPatcher.ts:0-0
Timestamp: 2025-06-05T14:28:34.813Z
Learning: For demo scripts in the lifinance/contracts repository, simpler validation (like checking for zero address) is acceptable and additional contract code validation is not necessary.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-12-17T10:28:34.379Z

Learnt from: CR
Repo: lifinance/contracts PR: 0
File: .cursor/rules/104-receiver-contracts.mdc:0-0
Timestamp: 2025-12-17T10:28:34.379Z
Learning: Applies to src/Periphery/Receiver*.sol : Receiver contract constructor must validate all parameters (check for `address(0)`) and revert with `InvalidConfig()` if invalid, initialize immutables, and call `WithdrawablePeriphery(_owner)`

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-07-17T04:21:55.549Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1266
File: script/deploy/safe/execute-pending-timelock-tx.ts:334-334
Timestamp: 2025-07-17T04:21:55.549Z
Learning: In the lifinance/contracts repository, 0xDEnYO prefers to keep private key processing simple in scripts like execute-pending-timelock-tx.ts without adding format validation, prioritizing code simplicity over strict validation in controlled environments.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-07-17T04:21:26.825Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1266
File: script/deploy/safe/execute-pending-timelock-tx.ts:627-628
Timestamp: 2025-07-17T04:21:26.825Z
Learning: In the lifinance/contracts repository, 0xDEnYO prefers to keep '0x0' as a fallback address in gas estimation calls rather than throwing errors when the wallet account address is not available, prioritizing code simplicity over strict validation.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-08-20T09:56:24.259Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1327
File: docs/OutputValidator.md:23-29
Timestamp: 2025-08-20T09:56:24.259Z
Learning: In src/Periphery/OutputValidator.sol, the ERC20 token flow that uses balanceOf(msg.sender) to compute actualAmount and transfers the excess via safeTransferFrom is intentional behavior, as confirmed by 0xDEnYO. The design choice to potentially include pre-existing ERC20 balances in the excess calculation is by design and should not be flagged as a security concern in future reviews.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-12-17T10:28:34.379Z

Learnt from: CR
Repo: lifinance/contracts PR: 0
File: .cursor/rules/104-receiver-contracts.mdc:0-0
Timestamp: 2025-12-17T10:28:34.379Z
Learning: Applies to src/Periphery/Receiver*.sol : Receiver contracts must use `LibAsset` for asset operations, `LibSwap` for swap data structures, and `SafeTransferLib` from solady for safe transfers

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2024-09-23T02:04:16.323Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 807
File: src/Periphery/GasZipPeriphery.sol:49-53
Timestamp: 2024-09-23T02:04:16.323Z
Learning: When `LibAsset.maxApproveERC20` is used with `type(uint256).max`, the team acknowledges and accepts the associated security risks. In future reviews, avoid flagging this as a concern.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-08-29T11:53:38.549Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: test/solidity/Periphery/LDA/BaseCoreRouteTest.t.sol:379-388
Timestamp: 2025-08-29T11:53:38.549Z
Learning: In test/solidity/Periphery/LDA/BaseCoreRouteTest.t.sol, for the revert-testing helper function _executeAndVerifySwap, only the aggregator branch (CommandType.DistributeSelfERC20) should use amountIn-1 to underfund and trigger insufficient balance errors, while user-funded branches should use the full amountIn to test other error conditions.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-08-26T12:11:21.073Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: src/Periphery/Lda/Facets/CoreRouteFacet.sol:4-6
Timestamp: 2025-08-26T12:11:21.073Z
Learning: In the lifinance/contracts repository, OpenZeppelin's SafeERC20 library provides safePermit functionality for IERC20Permit tokens. This is confirmed by working implementations in both CoreRouteFacet.sol and LiFiDEXAggregator.sol that use the pattern "using SafeERC20 for IERC20Permit;" and successfully call safePermit with permit parameters.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-08-26T12:11:21.073Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: src/Periphery/Lda/Facets/CoreRouteFacet.sol:4-6
Timestamp: 2025-08-26T12:11:21.073Z
Learning: OpenZeppelin's SafeERC20 library provides safePermit functionality for IERC20Permit tokens. The using SafeERC20 for IERC20Permit; directive enables calling safePermit on IERC20Permit tokens safely.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-08-26T12:11:21.073Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: src/Periphery/Lda/Facets/CoreRouteFacet.sol:4-6
Timestamp: 2025-08-26T12:11:21.073Z
Learning: OpenZeppelin's SafeERC20 library provides safePermit functionality for IERC20Permit tokens in modern versions. The usage pattern "using SafeERC20 for IERC20Permit;" is valid and enables safe permit operations. This is confirmed to be working in the lifinance/contracts codebase.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-12-17T10:28:34.379Z

Learnt from: CR
Repo: lifinance/contracts PR: 0
File: .cursor/rules/104-receiver-contracts.mdc:0-0
Timestamp: 2025-12-17T10:28:34.379Z
Learning: Applies to src/Periphery/Receiver*.sol : Receiver contracts must use try-catch for executor calls, always have fallback to send raw tokens on failure, and reset approvals to 0 after operations

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-08-27T23:01:41.042Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: src/Libraries/LibUniV3Logic.sol:4-5
Timestamp: 2025-08-27T23:01:41.042Z
Learning: In the lifinance/contracts repository, the pattern `import { IERC20, SafeERC20 } from "openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";` is correctly used throughout the codebase and works as expected. SafeERC20.sol re-exports IERC20 in modern OpenZeppelin versions, making this combined import valid. Do not suggest splitting these imports.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-12-17T10:28:03.630Z

Learnt from: CR
Repo: lifinance/contracts PR: 0
File: .cursor/rules/101-solidity-contracts.mdc:0-0
Timestamp: 2025-12-17T10:28:03.630Z
Learning: Applies to src/**/*.sol : Apply Diamond patterns with existing libs (LibAsset/LibSwap/LibAllowList, Validatable/SwapperV2); prefer parameters over `msg.sender` for refund addresses.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-08-26T12:11:21.073Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: src/Periphery/Lda/Facets/CoreRouteFacet.sol:4-6
Timestamp: 2025-08-26T12:11:21.073Z
Learning: OpenZeppelin's SafeERC20 library provides safePermit functionality for IERC20Permit tokens. This is confirmed by actual usage in the lifinance/contracts codebase where safePermit is used in multiple files including CoreRouteFacet.sol and LiFiDEXAggregator.sol. The pattern "using SafeERC20 for IERC20Permit;" is valid and working.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-01-23T02:26:29.900Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 946
File: config/networks.json:260-260
Timestamp: 2025-01-23T02:26:29.900Z
Learning: Each network in the configuration has its own unique wrapped native token address specified in the `wrappedNativeAddress` field. These addresses are network-specific and should not be expected to be the same or available across different networks.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-12-17T10:28:13.904Z

Learnt from: CR
Repo: lifinance/contracts PR: 0
File: .cursor/rules/102-facets.mdc:0-0
Timestamp: 2025-12-17T10:28:13.904Z
Learning: Applies to src/Facets/**/*Facet.sol : In Solidity facets with `minAmountOut` or similar bridge parameters, update the bridge's minAmountOut in `swapAndStartBridgeTokensVia{FacetName}` to account for positive slippage from swaps after `_depositAndSwap` updates `_bridgeData.minAmount`, adjusting proportionally for decimal differences if applicable

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2024-10-09T03:47:21.269Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 807
File: src/Periphery/GasZipPeriphery.sol:0-0
Timestamp: 2024-10-09T03:47:21.269Z
Learning: In the `depositToGasZipERC20` function, it's necessary to call `GasZipPeriphery(payable(address(this))).depositToGasZipNative{...}` to specify `msg.value` after the swap, as calling the internal function directly does not allow setting `msg.value`.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-08-28T22:41:58.814Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: test/solidity/Periphery/Lda/BaseCoreRouteTest.t.sol:387-395
Timestamp: 2025-08-28T22:41:58.814Z
Learning: In the lifinance/contracts repository, SafeERC20's safeApprove() method handles the two-step approval pattern internally when needed, so manual reset-to-zero followed by approval is unnecessary. Simply changing from IERC20.approve() to IERC20.safeApprove() provides the safety benefits.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-08-26T12:11:21.073Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: src/Periphery/Lda/Facets/CoreRouteFacet.sol:4-6
Timestamp: 2025-08-26T12:11:21.073Z
Learning: OpenZeppelin's SafeERC20 library provides safePermit functionality for IERC20Permit tokens. The pattern "using SafeERC20 for IERC20Permit;" enables calling safePermit on IERC20Permit tokens safely. This functionality is available in recent versions of OpenZeppelin contracts.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-12-17T10:28:40.640Z

Learnt from: CR
Repo: lifinance/contracts PR: 0
File: .cursor/rules/105-security.mdc:0-0
Timestamp: 2025-12-17T10:28:40.640Z
Learning: Applies to {src,script}/**/*.sol : Do not weaken existing access controls, timelock flows, or Safe multisig protections; any change that touches admin-only functionality must call out its governance impact

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-06-07T04:11:20.100Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1141
File: test/solidity/Periphery/LidoWrapper.t.sol:55-70
Timestamp: 2025-06-07T04:11:20.100Z
Learning: On Layer 2 networks, Lido's stETH contract has inverted function naming compared to expected behavior: `unwrap()` converts stETH → wstETH, and `wrap()` converts wstETH → stETH. This is the opposite of what the function names suggest but is how Lido implemented their L2 contracts.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-08-29T11:07:57.743Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: src/Periphery/LDA/Facets/CurveFacet.sol:87-91
Timestamp: 2025-08-29T11:07:57.743Z
Learning: In src/Periphery/LDA/Facets/CurveFacet.sol, modern Curve pools (isV2=true, representing V2/NG pools) should reject native tokenIn by adding an early revert check when LibAsset.isNativeAsset(tokenIn) is true, since ICurveV2 exchange functions are non-payable and cannot accept native ETH.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-08-27T23:36:40.773Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: test/solidity/Periphery/GasZipPeriphery.t.sol:223-233
Timestamp: 2025-08-27T23:36:40.773Z
Learning: In bridge facet swap flows with requiresDeposit=false, tokens are already held by the diamond contract from previous swaps. When subsequent contracts like GasZipPeriphery call LibAsset.depositAsset, they pull tokens from msg.sender (the diamond) to themselves via transferFrom, requiring no additional approvals since the diamond already owns the tokens.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-09-25T00:12:58.536Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1395
File: src/Periphery/FeeForwarder.sol:78-120
Timestamp: 2025-09-25T00:12:58.536Z
Learning: In src/Periphery/FeeForwarder.sol, the team intentionally uses address(this).balance for refunding remaining native tokens because the contract is designed to never hold funds and not collect dust, making it safe to return the entire balance to the caller.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-12-17T10:27:21.083Z

Learnt from: CR
Repo: lifinance/contracts PR: 0
File: .cursor/rules/002-architecture.mdc:0-0
Timestamp: 2025-12-17T10:27:21.083Z
Learning: Applies to **/*.sol : Delegate complex logic to libraries (LibAsset, LibSwap, LibAllowList, SwapperV2, Validatable) and helper contracts

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-12-12T11:16:09.236Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 1530
File: src/Interfaces/ICircleBridgeProxy.sol:20-29
Timestamp: 2025-12-12T11:16:09.236Z
Learning: In src/Interfaces/ICircleBridgeProxy.sol, ensure the ICircleBridgeProxy interface corresponds to Celer's CircleBridgeProxy contract (not Circle's native TokenMessenger). The depositForBurn signature must be: depositForBurn(uint256 _amount, uint64 _dstChid, bytes32 _mintRecipient, address _burnToken, uint256 _maxFee, uint32 _minFinalityThreshold) external. This change should be scoped specifically to this file; pattern is not needed since it targets a single file. This improves correctness by aligning the interface with the correct contract and parameter types (dst chain id as uint64, no bytes32 destinationCaller parameter).

Applied to files:

src/Periphery/TokenWrapper.sol

🔇 Additional comments (4)

src/Periphery/TokenWrapper.sol (4)

1-12: LGTM: License, imports, and pragma correctly updated.

The license change to LGPL-3.0-only aligns with coding guidelines, and the new imports (IERC20Metadata, IWrapper, error types) support the converter functionality properly. No blank line between license and pragma is correct per team conventions.

44-59: LGTM: Converter setup and swap ratio calculation.

The constructor correctly:

Validates converter is a contract when provided

Sets up max approval for gas optimization (accepted pattern per learnings)

Calculates SWAP_RATIO_MULTIPLIER based on wrapped token decimals for converter path

Uses 1:1 ratio for direct wrapping

The swap ratio multiplier approach was explicitly chosen over balance checks per past review discussion to avoid accepting arbitrary return values.

Note: The design assumes the converter contract is trusted to honor the exact SWAP_RATIO_MULTIPLIER ratio without verification. Ensure converter contracts are thoroughly vetted before deployment, as they have max approval and control the wrap/unwrap exchange rates.

67-71: LGTM: Deposit logic correctly implements converter-aware wrapping.

The deposit function properly:

Routes through converter when present

Calculates wrapped token amount using the precalculated ratio

Uses SafeTransferLib for safe transfers (addressing past feedback)

Math verified for both converter (decimal conversion) and direct wrapping (1:1) scenarios. The multiplication-then-division pattern correctly scales between native token (18 decimals) and wrapped token (variable decimals).

Note: For extremely small deposits (e.g., 1 wei) with low-decimal tokens (<18), precision loss could round down to 0 wrapped tokens while consuming the native amount. This is an acceptable edge case given gas costs far exceed these amounts.

76-92: LGTM: Withdraw logic correctly implements inverse conversion.

The withdraw function properly:

Pulls wrapped tokens from caller using SafeTransferLib.safeTransferFrom

Routes unwrap through converter

Calculates native amount as the inverse of deposit (correct ratio arithmetic)

Returns native tokens using SafeTransferLib.safeTransferETH

Math verified as the correct inverse of deposit for both converter and direct paths. The comment on lines 77-80 appropriately explains the balance-only optimization.

src/Periphery/TokenWrapper.sol

Replace NoTransferToNullAddress with InvalidConfig for _wrappedToken and _owner parameter validation to follow project's constructor parameter validation pattern.

…ontracts into feat/token-wrapper-converter

coderabbitai

Actionable comments posted: 0

♻️ Duplicate comments (1)

src/Periphery/TokenWrapper.sol (1)
14-17: Add fund custody documentation to contract NatSpec.

Per coding guidelines for bug bounty clarity, this contract should explicitly document that it is not designed to custody user funds. The contract only holds tokens/ETH transiently during wrap/unwrap operations.
📝 Suggested NatSpec addition
 /// @title TokenWrapper
 /// @author LI.FI (https://li.fi)
 /// @notice Provides functionality for wrapping and unwrapping tokens
+/// @notice This contract is not intended to custody user funds. Any funds held are transient during wrap/unwrap execution and should not persist.
 /// @custom:version 1.2.0
Based on coding guidelines.

🧹 Nitpick comments (1)

src/Periphery/TokenWrapper.sol (1)
24-25: Unused error declaration can be removed.

The WithdrawFailure error is declared but never used anywhere in the contract. Consider removing it to reduce bytecode size and improve code clarity.
🔧 Suggested fix
-    /// Errors ///
-    error WithdrawFailure();
-

📜 Review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between bc81b9c and dce0d42.

📒 Files selected for processing (2)

script/deploy/resources/deployRequirements.json
src/Periphery/TokenWrapper.sol

🚧 Files skipped from review as they are similar to previous changes (1)

script/deploy/resources/deployRequirements.json

🧰 Additional context used

📓 Path-based instructions (5)

src/Periphery/**/*.sol

📄 CodeRabbit inference engine (.cursor/rules/001-project-structure.mdc)

Place new periphery contracts in the src/Periphery/ directory. These are standalone contracts called by the Diamond but are not facets and can be upgraded independently.

Periphery contracts handle specialized flows that don't belong directly in the Diamond

Files:

src/Periphery/TokenWrapper.sol

**/*.sol

📄 CodeRabbit inference engine (.cursor/rules/001-project-structure.mdc)

Use remappings defined in remappings.txt for imports: lifi/ → src/, test/ → test/, and external libs like @openzeppelin/, solmate/, solady/, permit2/

**/*.sol: Single Diamond (EIP-2535) as main entrypoint for all protocol interactions
Delegate complex logic to libraries (LibAsset, LibSwap, LibAllowList, SwapperV2, Validatable) and helper contracts

After Solidity changes, run forge test (or note suites remaining)

**/*.sol: Own files must use // SPDX-License-Identifier: LGPL-3.0-only immediately followed by the pragma statement with no blank line in between
All contracts must use pragma solidity ^0.8.17;
Functions and variables use camelCase; constants and immutables are CONSTANT_CASE
Function parameters use leading underscore (e.g., _amount)
Contracts and interfaces must include NatSpec: @title (matching contract/interface name), @author LI.FI (https://li.fi), @notice describing purpose, and @custom:version X.Y.Z
Public and external functions require NatSpec including params and returns documentation
For pure test/script scaffolding keep NatSpec headers minimal but retain SPDX and pragma
Use single blank lines between logical sections and between function declarations
Follow in-function blank-line rules: blank lines before emits/returns; no stray gaps
Use custom errors instead of revert strings; prefer existing errors/helpers before adding new ones
Use generic errors from src/Errors/GenericErrors.sol; bump @custom:version when adding; facet-specific errors stay local
Adhere to rules in .solhint.json
Avoid assembly unless necessary and heavily commented with justification (why assembly is needed); prefer existing helpers over new implementations

Do not mix interface and implementation in the same file

Files:

src/Periphery/TokenWrapper.sol

src/**/*.sol

📄 CodeRabbit inference engine (.cursor/rules/101-solidity-contracts.mdc)

src/**/*.sol: NatSpec required on contracts/interfaces in src/: @title, @author LI.FI (https://li.fi), @notice, @custom:version X.Y.Z; document all public/external functions with params/returns.
Bug bounty clarity: In contract NatSpec, explicitly document whether the contract is intended to hold/custody funds. If it is not designed to hold funds, state that clearly (e.g., "This contract is not intended to custody user funds / hold balances; any funds held are incidental (e.g., transient during execution) and should not persist."). If it is designed to hold funds (e.g., FeeCollector), describe what funds it holds and under what conditions.
Apply Diamond patterns with existing libs (LibAsset/LibSwap/LibAllowList, Validatable/SwapperV2); prefer parameters over msg.sender for refund addresses.

src/**/*.sol: Follow gas-related best practices in existing facets before introducing new micro-optimizations in Solidity
Prefer using existing optimized libraries already in the repo (e.g., Solady/Solmate utilities) where they are in use, rather than hand-rolled low-level code
Do not sacrifice readability or safety for minor gas wins; explain any non-obvious optimization and justify it with expected impact
Avoid introducing inline assembly unless clearly needed; when used, document assumptions and invariants thoroughly

All contracts in src/ require audits except Interfaces in src/Interfaces/** (type definitions only) and external dependencies in lib/

Files:

src/Periphery/TokenWrapper.sol

{src,script,test}/**/*.{sol,ts}

📄 CodeRabbit inference engine (.cursor/rules/105-security.mdc)

Validate all external inputs and configuration (including script/env values) explicitly; prefer existing validation helpers (e.g., Validatable, config readers) over ad-hoc checks

Files:

src/Periphery/TokenWrapper.sol

{src,script}/**/*.sol

📄 CodeRabbit inference engine (.cursor/rules/105-security.mdc)

{src,script}/**/*.sol: Do not weaken existing access controls, timelock flows, or Safe multisig protections; any change that touches admin-only functionality must call out its governance impact
Avoid introducing new external call patterns in facets or scripts without checking existing libraries (LibAsset, LibSwap, LibAllowList) for prior art

Files:

src/Periphery/TokenWrapper.sol

🧠 Learnings (53)

📓 Common learnings

Learnt from: ezynda3
Repo: lifinance/contracts PR: 1124
File: script/demoScripts/utils/lib/cowShedSdk.ts:0-0
Timestamp: 2025-06-05T14:50:17.275Z
Learning: For demo scripts and example code, ezynda3 prefers to keep the code simple and focused on demonstrating the core functionality rather than adding extensive input validation or defensive programming measures.

📚 Learning: 2024-10-14T08:23:38.076Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 831
File: src/Helpers/WithdrawablePeriphery.sol:26-30
Timestamp: 2024-10-14T08:23:38.076Z
Learning: In the `WithdrawablePeriphery.sol` contract, when transferring Ether in the `withdrawToken` function, prefer using `SafeTransferLib.safeTransferETH` instead of low-level `call` for safer Ether transfers.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-12-17T10:28:34.379Z

Learnt from: CR
Repo: lifinance/contracts PR: 0
File: .cursor/rules/104-receiver-contracts.mdc:0-0
Timestamp: 2025-12-17T10:28:34.379Z
Learning: Applies to src/Periphery/Receiver*.sol : Receiver contracts must implement internal `_swapAndCompleteBridgeTokens()` function that handles native vs ERC20 tokens: use `safeApproveWithRetry()` or `safeIncreaseAllowance()` for ERC20 (reset to 0 after), pass `value: amount` for native tokens, call `executor.swapAndCompleteBridgeTokens()` in try-catch block, and on failure send raw tokens to receiver and emit `LiFiTransferRecovered`

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-08-29T11:18:56.656Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: src/Periphery/LDA/Facets/NativeWrapperFacet.sol:1-2
Timestamp: 2025-08-29T11:18:56.656Z
Learning: For src/Periphery/LDA/Facets/NativeWrapperFacet.sol, the SPDX license identifier should be immediately followed by the pragma statement without a blank line in between, as confirmed by mirooon.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-11-04T16:27:25.773Z

Learnt from: RnkSngh
Repo: lifinance/contracts PR: 1422
File: src/Interfaces/ITokenMessenger.sol:1-3
Timestamp: 2025-11-04T16:27:25.773Z
Learning: For external/third-party interfaces in src/Interfaces/ (e.g., ITokenMessenger.sol for Polymer/CCTP), the team does not require following the project's NatSpec, licensing (LGPL-3.0-only), or pragma conventions, as these interfaces represent contracts the team doesn't control.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2024-09-23T01:48:39.325Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 807
File: src/Periphery/GasZipPeriphery.sol:29-32
Timestamp: 2024-09-23T01:48:39.325Z
Learning: In our codebase, input validation for zero addresses in constructor parameters is not required because our deployment flow ensures that contracts cannot be deployed with zero address parameters unless we are okay with it.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-06-16T09:28:18.027Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1207
File: script/deploy/zksync/DeployPioneerFacet.zksync.s.sol:29-37
Timestamp: 2025-06-16T09:28:18.027Z
Learning: In the LIFI contracts codebase, constructor arguments in Solidity deployment scripts do not need validation because they are already validated by bash scripts in combination with the deployRequirements.json file. The deployRequirements.json file contains validation rules like "allowToDeployWithZeroAddress": false to prevent invalid constructor arguments during deployment.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-06-24T08:13:01.516Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1234
File: script/deploy/facets/DeployRelayFacet.s.sol:31-34
Timestamp: 2025-06-24T08:13:01.516Z
Learning: In the lifinance/contracts repository, parameter validation (like checking for address(0) or missing configuration values) should not be suggested in Foundry deploy scripts (files in script/deploy/) because the deployment process includes pre-validation through deployRequirements.json that checks these parameters before the Foundry scripts are executed. The deploy scripts can safely assume parameters are valid.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-02-11T10:33:52.791Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 988
File: script/deploy/facets/DeployPermit2Proxy.s.sol:33-37
Timestamp: 2025-02-11T10:33:52.791Z
Learning: The Permit2Proxy contract must not be deployed with zero addresses for its critical dependencies (LiFiDiamond, permit2Address, safeAddress). This is enforced by passing `false` to `_getConfigContractAddress` function.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-07-17T04:21:50.790Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1266
File: script/deploy/safe/execute-pending-timelock-tx.ts:129-129
Timestamp: 2025-07-17T04:21:50.790Z
Learning: In the lifinance/contracts repository, 0xDEnYO prefers not to add private key format validation in deployment scripts like execute-pending-timelock-tx.ts, prioritizing code simplicity over strict validation in their controlled environment.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-08-29T11:12:02.184Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: src/Periphery/LDA/Facets/SyncSwapV2Facet.sol:33-38
Timestamp: 2025-08-29T11:12:02.184Z
Learning: In LDA facets (src/Periphery/Lda/Facets/**/*.sol), tokenIn parameter validation (checking for address(0)) is intentionally not performed as part of the design architecture.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-07-16T01:03:08.106Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1275
File: src/Facets/AllBridgeFacet.sol:164-164
Timestamp: 2025-07-16T01:03:08.106Z
Learning: In src/Facets/AllBridgeFacet.sol, the team has decided that explicit validation for address downcasting from `_bridgeData.sendingAssetId` to `bytes32(uint256(uint160(_bridgeData.sendingAssetId)))` is not required, accepting the potential risk of silent overflow from unsafe downcasting.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-06-05T14:28:34.813Z

Learnt from: ezynda3
Repo: lifinance/contracts PR: 1124
File: script/demoScripts/demoPatcher.ts:0-0
Timestamp: 2025-06-05T14:28:34.813Z
Learning: For demo scripts in the lifinance/contracts repository, simpler validation (like checking for zero address) is acceptable and additional contract code validation is not necessary.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-12-17T10:28:34.379Z

Learnt from: CR
Repo: lifinance/contracts PR: 0
File: .cursor/rules/104-receiver-contracts.mdc:0-0
Timestamp: 2025-12-17T10:28:34.379Z
Learning: Applies to src/Periphery/Receiver*.sol : Receiver contract constructor must validate all parameters (check for `address(0)`) and revert with `InvalidConfig()` if invalid, initialize immutables, and call `WithdrawablePeriphery(_owner)`

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-07-17T04:21:55.549Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1266
File: script/deploy/safe/execute-pending-timelock-tx.ts:334-334
Timestamp: 2025-07-17T04:21:55.549Z
Learning: In the lifinance/contracts repository, 0xDEnYO prefers to keep private key processing simple in scripts like execute-pending-timelock-tx.ts without adding format validation, prioritizing code simplicity over strict validation in controlled environments.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-07-17T04:21:26.825Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1266
File: script/deploy/safe/execute-pending-timelock-tx.ts:627-628
Timestamp: 2025-07-17T04:21:26.825Z
Learning: In the lifinance/contracts repository, 0xDEnYO prefers to keep '0x0' as a fallback address in gas estimation calls rather than throwing errors when the wallet account address is not available, prioritizing code simplicity over strict validation.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-08-20T09:56:24.259Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1327
File: docs/OutputValidator.md:23-29
Timestamp: 2025-08-20T09:56:24.259Z
Learning: In src/Periphery/OutputValidator.sol, the ERC20 token flow that uses balanceOf(msg.sender) to compute actualAmount and transfers the excess via safeTransferFrom is intentional behavior, as confirmed by 0xDEnYO. The design choice to potentially include pre-existing ERC20 balances in the excess calculation is by design and should not be flagged as a security concern in future reviews.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-12-17T10:28:34.379Z

Learnt from: CR
Repo: lifinance/contracts PR: 0
File: .cursor/rules/104-receiver-contracts.mdc:0-0
Timestamp: 2025-12-17T10:28:34.379Z
Learning: Applies to src/Periphery/Receiver*.sol : Receiver contracts must use `LibAsset` for asset operations, `LibSwap` for swap data structures, and `SafeTransferLib` from solady for safe transfers

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2024-09-23T02:04:16.323Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 807
File: src/Periphery/GasZipPeriphery.sol:49-53
Timestamp: 2024-09-23T02:04:16.323Z
Learning: When `LibAsset.maxApproveERC20` is used with `type(uint256).max`, the team acknowledges and accepts the associated security risks. In future reviews, avoid flagging this as a concern.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-08-29T11:53:38.549Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: test/solidity/Periphery/LDA/BaseCoreRouteTest.t.sol:379-388
Timestamp: 2025-08-29T11:53:38.549Z
Learning: In test/solidity/Periphery/LDA/BaseCoreRouteTest.t.sol, for the revert-testing helper function _executeAndVerifySwap, only the aggregator branch (CommandType.DistributeSelfERC20) should use amountIn-1 to underfund and trigger insufficient balance errors, while user-funded branches should use the full amountIn to test other error conditions.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-08-26T12:11:21.073Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: src/Periphery/Lda/Facets/CoreRouteFacet.sol:4-6
Timestamp: 2025-08-26T12:11:21.073Z
Learning: In the lifinance/contracts repository, OpenZeppelin's SafeERC20 library provides safePermit functionality for IERC20Permit tokens. This is confirmed by working implementations in both CoreRouteFacet.sol and LiFiDEXAggregator.sol that use the pattern "using SafeERC20 for IERC20Permit;" and successfully call safePermit with permit parameters.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-08-26T12:11:21.073Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: src/Periphery/Lda/Facets/CoreRouteFacet.sol:4-6
Timestamp: 2025-08-26T12:11:21.073Z
Learning: OpenZeppelin's SafeERC20 library provides safePermit functionality for IERC20Permit tokens. The using SafeERC20 for IERC20Permit; directive enables calling safePermit on IERC20Permit tokens safely.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-08-26T12:11:21.073Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: src/Periphery/Lda/Facets/CoreRouteFacet.sol:4-6
Timestamp: 2025-08-26T12:11:21.073Z
Learning: OpenZeppelin's SafeERC20 library provides safePermit functionality for IERC20Permit tokens in modern versions. The usage pattern "using SafeERC20 for IERC20Permit;" is valid and enables safe permit operations. This is confirmed to be working in the lifinance/contracts codebase.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-12-17T10:28:34.379Z

Learnt from: CR
Repo: lifinance/contracts PR: 0
File: .cursor/rules/104-receiver-contracts.mdc:0-0
Timestamp: 2025-12-17T10:28:34.379Z
Learning: Applies to src/Periphery/Receiver*.sol : Receiver contracts must use try-catch for executor calls, always have fallback to send raw tokens on failure, and reset approvals to 0 after operations

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-08-27T23:01:41.042Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: src/Libraries/LibUniV3Logic.sol:4-5
Timestamp: 2025-08-27T23:01:41.042Z
Learning: In the lifinance/contracts repository, the pattern `import { IERC20, SafeERC20 } from "openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";` is correctly used throughout the codebase and works as expected. SafeERC20.sol re-exports IERC20 in modern OpenZeppelin versions, making this combined import valid. Do not suggest splitting these imports.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-12-17T10:28:03.630Z

Learnt from: CR
Repo: lifinance/contracts PR: 0
File: .cursor/rules/101-solidity-contracts.mdc:0-0
Timestamp: 2025-12-17T10:28:03.630Z
Learning: Applies to src/**/*.sol : Apply Diamond patterns with existing libs (LibAsset/LibSwap/LibAllowList, Validatable/SwapperV2); prefer parameters over `msg.sender` for refund addresses.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-08-26T12:11:21.073Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: src/Periphery/Lda/Facets/CoreRouteFacet.sol:4-6
Timestamp: 2025-08-26T12:11:21.073Z
Learning: OpenZeppelin's SafeERC20 library provides safePermit functionality for IERC20Permit tokens. This is confirmed by actual usage in the lifinance/contracts codebase where safePermit is used in multiple files including CoreRouteFacet.sol and LiFiDEXAggregator.sol. The pattern "using SafeERC20 for IERC20Permit;" is valid and working.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-01-23T02:26:29.900Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 946
File: config/networks.json:260-260
Timestamp: 2025-01-23T02:26:29.900Z
Learning: Each network in the configuration has its own unique wrapped native token address specified in the `wrappedNativeAddress` field. These addresses are network-specific and should not be expected to be the same or available across different networks.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-12-17T10:28:13.904Z

Learnt from: CR
Repo: lifinance/contracts PR: 0
File: .cursor/rules/102-facets.mdc:0-0
Timestamp: 2025-12-17T10:28:13.904Z
Learning: Applies to src/Facets/**/*Facet.sol : In Solidity facets with `minAmountOut` or similar bridge parameters, update the bridge's minAmountOut in `swapAndStartBridgeTokensVia{FacetName}` to account for positive slippage from swaps after `_depositAndSwap` updates `_bridgeData.minAmount`, adjusting proportionally for decimal differences if applicable

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2024-10-09T03:47:21.269Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 807
File: src/Periphery/GasZipPeriphery.sol:0-0
Timestamp: 2024-10-09T03:47:21.269Z
Learning: In the `depositToGasZipERC20` function, it's necessary to call `GasZipPeriphery(payable(address(this))).depositToGasZipNative{...}` to specify `msg.value` after the swap, as calling the internal function directly does not allow setting `msg.value`.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-08-28T22:41:58.814Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: test/solidity/Periphery/Lda/BaseCoreRouteTest.t.sol:387-395
Timestamp: 2025-08-28T22:41:58.814Z
Learning: In the lifinance/contracts repository, SafeERC20's safeApprove() method handles the two-step approval pattern internally when needed, so manual reset-to-zero followed by approval is unnecessary. Simply changing from IERC20.approve() to IERC20.safeApprove() provides the safety benefits.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-08-26T12:11:21.073Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: src/Periphery/Lda/Facets/CoreRouteFacet.sol:4-6
Timestamp: 2025-08-26T12:11:21.073Z
Learning: OpenZeppelin's SafeERC20 library provides safePermit functionality for IERC20Permit tokens. The pattern "using SafeERC20 for IERC20Permit;" enables calling safePermit on IERC20Permit tokens safely. This functionality is available in recent versions of OpenZeppelin contracts.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-12-17T10:28:40.640Z

Learnt from: CR
Repo: lifinance/contracts PR: 0
File: .cursor/rules/105-security.mdc:0-0
Timestamp: 2025-12-17T10:28:40.640Z
Learning: Applies to {src,script}/**/*.sol : Do not weaken existing access controls, timelock flows, or Safe multisig protections; any change that touches admin-only functionality must call out its governance impact

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-12-17T10:28:03.630Z

Learnt from: CR
Repo: lifinance/contracts PR: 0
File: .cursor/rules/101-solidity-contracts.mdc:0-0
Timestamp: 2025-12-17T10:28:03.630Z
Learning: Applies to src/**/*.sol : Bug bounty clarity: In contract NatSpec, explicitly document whether the contract is intended to hold/custody funds. If it is **not** designed to hold funds, state that clearly (e.g., "This contract is not intended to custody user funds / hold balances; any funds held are incidental (e.g., transient during execution) and should not persist."). If it _is_ designed to hold funds (e.g., `FeeCollector`), describe what funds it holds and under what conditions.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-12-17T10:28:03.630Z

Learnt from: CR
Repo: lifinance/contracts PR: 0
File: .cursor/rules/101-solidity-contracts.mdc:0-0
Timestamp: 2025-12-17T10:28:03.630Z
Learning: Applies to src/**/*.sol : NatSpec required on contracts/interfaces in `src/`: `title`, `author LI.FI (https://li.fi)`, `notice`, `custom:version X.Y.Z`; document all public/external functions with params/returns.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-12-17T10:27:50.916Z

Learnt from: CR
Repo: lifinance/contracts PR: 0
File: .cursor/rules/100-solidity-basics.mdc:0-0
Timestamp: 2025-12-17T10:27:50.916Z
Learning: Applies to **/*.sol : Contracts and interfaces must include NatSpec: `title` (matching contract/interface name), `author LI.FI (https://li.fi)`, `notice` describing purpose, and `custom:version X.Y.Z`

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-12-17T10:28:21.255Z

Learnt from: CR
Repo: lifinance/contracts PR: 0
File: .cursor/rules/103-solidity-interfaces.mdc:0-0
Timestamp: 2025-12-17T10:28:21.255Z
Learning: Applies to src/Interfaces/**/*.sol : When copying from or shadowing an external protocol, add a short NatSpec note with a link or identifier that references the original contract or documentation

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2024-10-09T03:47:21.269Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 807
File: src/Interfaces/IGasZip.sol:19-19
Timestamp: 2024-10-09T03:47:21.269Z
Learning: In `IGasZip.sol`, adding NatSpec documentation for the `deposit` function is not required.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-08-20T18:10:04.344Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: test/solidity/Periphery/Lda/Facets/VelodromeV2Facet.t.sol:886-903
Timestamp: 2025-08-20T18:10:04.344Z
Learning: Mock contracts used for testing in the lifinance/contracts repository do not require NatSpec comments, as they are internal testing utilities rather than production code.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-08-26T22:29:29.965Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: script/deploy/facets/LDA/DeployCurveFacet.s.sol:10-12
Timestamp: 2025-08-26T22:29:29.965Z
Learning: Deployment scripts in the lifinance/contracts repository do not require NatSpec documentation, even for public/external functions. This is an exception to the general NatSpec requirement for public/external functions in the codebase.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-12-17T10:27:50.916Z

Learnt from: CR
Repo: lifinance/contracts PR: 0
File: .cursor/rules/100-solidity-basics.mdc:0-0
Timestamp: 2025-12-17T10:27:50.916Z
Learning: Applies to **/*.sol : Public and external functions require NatSpec including params and returns documentation

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2024-10-10T03:18:20.721Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 782
File: test/solidity/Helpers/WithdrawablePeriphery.t.sol:22-35
Timestamp: 2024-10-10T03:18:20.721Z
Learning: In Solidity tests for withdrawal functions in `test/solidity/Helpers/WithdrawablePeriphery.t.sol`, do not suggest adding tests where the withdrawal amount exceeds the contract's balance, as such tests are unnecessary because any contract will fail in that case.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2024-10-09T03:47:21.269Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 807
File: src/Periphery/GasZipPeriphery.sol:4-14
Timestamp: 2024-10-09T03:47:21.269Z
Learning: In `GasZipPeriphery.sol`, `LibUtil` and `Validatable` are used, so ensure not to suggest their removal in future reviews.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-09-19T14:10:55.064Z

Learnt from: ezynda3
Repo: lifinance/contracts PR: 1324
File: src/Facets/EcoFacet.sol:68-71
Timestamp: 2025-09-19T14:10:55.064Z
Learning: In src/Facets/EcoFacet.sol, the team prefers to use msg.sender as the refund address for native tokens in the refundExcessNative modifier, contrary to the general guideline of requiring explicit refund parameters.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-04-22T09:04:44.244Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1112
File: deployments/soneium.diamond.json:81-81
Timestamp: 2025-04-22T09:04:44.244Z
Learning: In the lifinance/contracts repository, it's normal and expected for periphery contracts to have empty address values in deployment files when they are not deployed on a particular network. This applies to all periphery contracts including ReceiverChainflip, ReceiverStargateV2, and others. PRs should not flag empty periphery contract addresses as issues.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-02-24T09:35:34.908Z

Learnt from: ezynda3
Repo: lifinance/contracts PR: 984
File: src/Periphery/ReceiverChainflip.sol:48-62
Timestamp: 2025-02-24T09:35:34.908Z
Learning: Contract address validations for periphery contracts like ReceiverChainflip are handled in deployment scripts rather than in the constructor.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-12-17T10:28:34.379Z

Learnt from: CR
Repo: lifinance/contracts PR: 0
File: .cursor/rules/104-receiver-contracts.mdc:0-0
Timestamp: 2025-12-17T10:28:34.379Z
Learning: Applies to src/Periphery/Receiver*.sol : For native tokens with gas checks (e.g., StargateV2), Receiver contracts must check `gasleft()` before executing; if insufficient gas, send tokens directly to receiver and emit `LiFiTransferRecovered`

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-06-07T04:11:20.100Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1141
File: test/solidity/Periphery/LidoWrapper.t.sol:55-70
Timestamp: 2025-06-07T04:11:20.100Z
Learning: On Layer 2 networks, Lido's stETH contract has inverted function naming compared to expected behavior: `unwrap()` converts stETH → wstETH, and `wrap()` converts wstETH → stETH. This is the opposite of what the function names suggest but is how Lido implemented their L2 contracts.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-08-29T11:07:57.743Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: src/Periphery/LDA/Facets/CurveFacet.sol:87-91
Timestamp: 2025-08-29T11:07:57.743Z
Learning: In src/Periphery/LDA/Facets/CurveFacet.sol, modern Curve pools (isV2=true, representing V2/NG pools) should reject native tokenIn by adding an early revert check when LibAsset.isNativeAsset(tokenIn) is true, since ICurveV2 exchange functions are non-payable and cannot accept native ETH.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-12-17T10:27:21.083Z

Learnt from: CR
Repo: lifinance/contracts PR: 0
File: .cursor/rules/002-architecture.mdc:0-0
Timestamp: 2025-12-17T10:27:21.083Z
Learning: Applies to **/*.sol : Delegate complex logic to libraries (LibAsset, LibSwap, LibAllowList, SwapperV2, Validatable) and helper contracts

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-08-27T23:36:40.773Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: test/solidity/Periphery/GasZipPeriphery.t.sol:223-233
Timestamp: 2025-08-27T23:36:40.773Z
Learning: In bridge facet swap flows with requiresDeposit=false, tokens are already held by the diamond contract from previous swaps. When subsequent contracts like GasZipPeriphery call LibAsset.depositAsset, they pull tokens from msg.sender (the diamond) to themselves via transferFrom, requiring no additional approvals since the diamond already owns the tokens.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-09-25T00:12:58.536Z

Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1395
File: src/Periphery/FeeForwarder.sol:78-120
Timestamp: 2025-09-25T00:12:58.536Z
Learning: In src/Periphery/FeeForwarder.sol, the team intentionally uses address(this).balance for refunding remaining native tokens because the contract is designed to never hold funds and not collect dust, making it safe to return the entire balance to the caller.

Applied to files:

src/Periphery/TokenWrapper.sol

📚 Learning: 2025-12-12T11:16:09.236Z

Learnt from: mirooon
Repo: lifinance/contracts PR: 1530
File: src/Interfaces/ICircleBridgeProxy.sol:20-29
Timestamp: 2025-12-12T11:16:09.236Z
Learning: In src/Interfaces/ICircleBridgeProxy.sol, ensure the ICircleBridgeProxy interface corresponds to Celer's CircleBridgeProxy contract (not Circle's native TokenMessenger). The depositForBurn signature must be: depositForBurn(uint256 _amount, uint64 _dstChid, bytes32 _mintRecipient, address _burnToken, uint256 _maxFee, uint32 _minFinalityThreshold) external. This change should be scoped specifically to this file; pattern is not needed since it targets a single file. This improves correctness by aligning the interface with the correct contract and parameter types (dst chain id as uint64, no bytes32 destinationCaller parameter).

Applied to files:

src/Periphery/TokenWrapper.sol

🪛 GitHub Actions: VersionControlAndAuditVerification

src/Periphery/TokenWrapper.sol

[error] 1-1: Could not find any logged audit for contract TokenWrapper in version 1.2.0.

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)

GitHub Check: enforce-min-test-coverage
GitHub Check: run-unit-tests

🔇 Additional comments (5)

src/Periphery/TokenWrapper.sol (5)
56-59: Verify 18-decimal assumption for non-converter wrapped tokens.

When no converter is used, SWAP_RATIO_MULTIPLIER is set to 1 ether (10^18), implying a 1:1 ratio with 18 decimals. This works correctly for standard WETH9 but would produce incorrect amounts if deployed with a wrapped token having different decimals.

Consider fetching decimals unconditionally to handle edge cases:
🔧 Optional defensive fix
         } else {
             CONVERTER = _wrappedToken;
-            SWAP_RATIO_MULTIPLIER = 1 ether; // 1:1 ratio for 18 decimals
+            // For non-converter case, fetch decimals to handle non-18 decimal tokens
+            uint8 wrappedDecimals = IERC20Metadata(_wrappedToken).decimals();
+            SWAP_RATIO_MULTIPLIER = 10 ** wrappedDecimals;
         }
67-71: LGTM - Deposit logic is correct.

The deposit function correctly handles both converter and non-converter paths. The SWAP_RATIO_MULTIPLIER approach (discussed in past reviews) provides a gas-efficient ratio calculation at the cost of potential precision loss for very small amounts with low-decimal tokens, which is acceptable given the use case.

76-92: Withdraw logic is correct; verify converter behavior.

The implementation correctly handles the withdrawal flow with proper use of SafeTransferLib. The balance-only approach (as noted in the comment) is a reasonable gas optimization given the use case.

Note: The contract trusts that IWrapper(CONVERTER).withdraw(amount) will deposit the expected native amount. If a malicious or buggy converter is configured, the safeTransferETH call at line 91 could either fail (insufficient balance) or transfer an incorrect amount. This is acceptable given converters are configured at deployment by trusted deployers.

94-95: LGTM - receive() is required for unwrap flow.

The payable receive function is necessary to accept native tokens from the converter/wrapper during withdraw() operations.

17-17: Audit required for version 1.2.0.

The pipeline correctly flags that no audit exists for TokenWrapper v1.2.0. The PR is appropriately labeled with AuditRequired. Ensure the audit is completed before deploying this version to production.

add converter parameter to TokenWrapper for WETH9-compatible interfac…

6c63ef9

…e support

lifi-action-bot marked this pull request as draft December 11, 2025 15:00

lifi-action-bot changed the title ~~add converter parameter to TokenWrapper for WETH9-compatible interfac…~~ add converter parameter to TokenWrapper for WETH9-compatible interfac… [TokenWrapper v1.2.0] Dec 11, 2025

lifi-action-bot added the AuditRequired label Dec 11, 2025

github-advanced-security bot found potential problems Dec 11, 2025

View reviewed changes

src/Periphery/TokenWrapper.sol Fixed Show fixed Hide fixed

coderabbitai bot reviewed Dec 11, 2025

View reviewed changes

optimize TokenWrapper with immutable variables and balance checks for…

633c97f

… converter support

coderabbitai bot reviewed Dec 11, 2025

View reviewed changes

remove useless approval

0379dff

github-advanced-security bot found potential problems Dec 12, 2025

View reviewed changes

src/Periphery/TokenWrapper.sol Fixed Show fixed Hide fixed

coderabbitai bot reviewed Dec 12, 2025

View reviewed changes

src/Periphery/TokenWrapper.sol Outdated Show resolved Hide resolved

src/Periphery/TokenWrapper.sol Outdated Show resolved Hide resolved

ezynda3 and others added 2 commits December 12, 2025 13:33

safe transfer

ecb8b1c

Merge branch 'main' into feat/token-wrapper-converter

a903247

ezynda3 marked this pull request as ready for review December 12, 2025 10:35

lifi-action-bot marked this pull request as draft December 12, 2025 10:36

ezynda3 marked this pull request as ready for review December 12, 2025 10:55

lifi-action-bot marked this pull request as draft December 12, 2025 10:55

ezynda3 marked this pull request as ready for review December 12, 2025 10:57

ezynda3 and others added 3 commits December 15, 2025 12:16

Update src/Periphery/TokenWrapper.sol

5a6ab81

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

Merge branch 'main' into feat/token-wrapper-converter

2d5148b

fix

3ffe919

lifi-action-bot changed the title ~~add converter parameter to TokenWrapper for WETH9-compatible interfac… [TokenWrapper v1.2.0]~~ add converter parameter to TokenWrapper for WETH9-compatible interfac… [IWrapper v1.0.0, TokenWrapper v1.2.0] Dec 15, 2025

coderabbitai bot reviewed Dec 15, 2025

View reviewed changes

0xDEnYO reviewed Dec 16, 2025

View reviewed changes

ezynda3 added 2 commits December 16, 2025 12:41

fixes

dcb8a6d

fixes

8843234

ezynda3 commented Dec 16, 2025

View reviewed changes

fixes

d364b8b

github-advanced-security bot found potential problems Dec 16, 2025

View reviewed changes

src/Periphery/TokenWrapper.sol Dismissed Show dismissed Hide dismissed

ezynda3 and others added 2 commits December 16, 2025 13:36

Merge branch 'main' into feat/token-wrapper-converter

813ada6

fixes

59c8de4

0xDEnYO reviewed Dec 17, 2025

View reviewed changes

coderabbitai bot reviewed Dec 22, 2025

View reviewed changes

script/deploy/facets/DeployTokenWrapper.s.sol Outdated Show resolved Hide resolved

ezynda3 and others added 4 commits December 22, 2025 16:37

Update script/deploy/facets/DeployTokenWrapper.s.sol

19f5d45

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

remove stable

1fb5247

Merge branch 'feat/token-wrapper-converter' of github.com:lifinance/c…

b3b2dba

…ontracts into feat/token-wrapper-converter

coderabbitai bot reviewed Dec 22, 2025

View reviewed changes

src/Periphery/TokenWrapper.sol Show resolved Hide resolved

src/Periphery/TokenWrapper.sol Outdated Show resolved Hide resolved

ezynda3 and others added 3 commits December 22, 2025 17:17

Merge branch 'main' into feat/token-wrapper-converter

9a49316

refactor: use InvalidConfig for constructor validation in TokenWrapper

4f4f19d

Replace NoTransferToNullAddress with InvalidConfig for _wrappedToken and _owner parameter validation to follow project's constructor parameter validation pattern.

Merge branch 'feat/token-wrapper-converter' of github.com:lifinance/c…

dce0d42

…ontracts into feat/token-wrapper-converter

coderabbitai bot reviewed Dec 22, 2025

View reviewed changes

add converter parameter to TokenWrapper for WETH9-compatible interfac… [IWrapper v1.0.0, TokenWrapper v1.2.0] #1531

Are you sure you want to change the base?

add converter parameter to TokenWrapper for WETH9-compatible interfac… [IWrapper v1.0.0, TokenWrapper v1.2.0] #1531

Conversation

ezynda3 commented Dec 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which Jira task belongs to this PR?

Why did I implement it this way?

Checklist before requesting a review

Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)

Uh oh!

coderabbitai bot commented Dec 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Pre-merge checks and finishing touches

Review ran into problems

Uh oh!

Uh oh!

lifi-action-bot commented Dec 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Coverage Report

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lifi-action-bot commented Dec 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🤖 GitHub Action: Security Alerts Review 🔍

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ezynda3 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

0xDEnYO Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

ezynda3 Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

mirooon Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

0xDEnYO Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

ezynda3 Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

0xDEnYO Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

ezynda3 commented Dec 11, 2025 •

edited

Loading

coderabbitai bot commented Dec 11, 2025 •

edited

Loading

lifi-action-bot commented Dec 11, 2025 •

edited

Loading

lifi-action-bot commented Dec 12, 2025 •

edited

Loading